AU: DeltaPerformer performs the download size/hash check now.
DownloadAction still calculates a hash in this case, however,
we can remove it when we remove old-style download support.
BUG=7393
TEST=unit tests, gmerge on device and updated with signed load,
with different download size/hash.
Change-Id: I5af9a09f87264159fc55070735463ad920fd7373
Review URL: http://codereview.chromium.org/3547019
diff --git a/delta_performer.cc b/delta_performer.cc
index 92d4733..f807665 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -190,12 +190,11 @@
}
// Remove protobuf and header info from buffer_, so buffer_ contains
// just data blobs
- size_t metadata_size = strlen(kDeltaMagic) + kDeltaVersionLength +
+ manifest_metadata_size_ = strlen(kDeltaMagic) + kDeltaVersionLength +
kDeltaProtobufLengthLength + protobuf_length;
- DiscardBufferHeadBytes(metadata_size,
- true); // do_hash
+ DiscardBufferHeadBytes(manifest_metadata_size_);
LOG_IF(WARNING, !prefs_->SetInt64(kPrefsManifestMetadataSize,
- metadata_size))
+ manifest_metadata_size_))
<< "Unable to save the manifest metadata size.";
manifest_valid_ = true;
block_size_ = manifest_.block_size();
@@ -281,8 +280,8 @@
CHECK_EQ(buffer_offset_, operation.data_offset());
CHECK_GE(buffer_.size(), operation.data_length());
- // Don't include the signature data blob in the hash.
- bool do_hash = !ExtractSignatureMessage(operation);
+ // Extract the signature message if it's in this operation.
+ ExtractSignatureMessage(operation);
DirectExtentWriter direct_writer;
ZeroPadExtentWriter zero_pad_writer(&direct_writer);
@@ -315,7 +314,7 @@
// Update buffer
buffer_offset_ += operation.data_length();
- DiscardBufferHeadBytes(operation.data_length(), do_hash);
+ DiscardBufferHeadBytes(operation.data_length());
return true;
}
@@ -458,8 +457,7 @@
// Update buffer.
buffer_offset_ += operation.data_length();
- DiscardBufferHeadBytes(operation.data_length(),
- true); // do_hash
+ DiscardBufferHeadBytes(operation.data_length());
return true;
}
@@ -479,20 +477,40 @@
signatures_message_data_.begin(),
buffer_.begin(),
buffer_.begin() + manifest_.signatures_size());
+ // The hash of all data consumed so far should be verified against the signed
+ // hash.
+ signed_hash_context_ = hash_calculator_.GetContext();
+ LOG_IF(WARNING, !prefs_->SetString(kPrefsUpdateStateSignedSHA256Context,
+ signed_hash_context_))
+ << "Unable to store the signed hash context.";
LOG(INFO) << "Extracted signature data of size "
<< manifest_.signatures_size() << " at "
<< manifest_.signatures_offset();
return true;
}
-bool DeltaPerformer::VerifyPayload(const string& public_key_path) {
+bool DeltaPerformer::VerifyPayload(
+ const string& public_key_path,
+ const std::string& update_check_response_hash,
+ const uint64_t update_check_response_size) {
string key_path = public_key_path;
if (key_path.empty()) {
key_path = kUpdatePayloadPublicKeyPath;
}
LOG(INFO) << "Verifying delta payload. Public key path: " << key_path;
+
+ // Verifies the download hash.
+ const string& download_hash_data = hash_calculator_.hash();
+ TEST_AND_RETURN_FALSE(!download_hash_data.empty());
+ TEST_AND_RETURN_FALSE(download_hash_data == update_check_response_hash);
+
+ // Verifies the download size.
+ TEST_AND_RETURN_FALSE(update_check_response_size ==
+ manifest_metadata_size_ + buffer_offset_);
+
+ // Verifies the signed payload hash.
if (!utils::FileExists(key_path.c_str())) {
- LOG(WARNING) << "Not verifying delta payload due to missing public key.";
+ LOG(WARNING) << "Not verifying signed delta payload -- missing public key.";
return true;
}
TEST_AND_RETURN_FALSE(!signatures_message_data_.empty());
@@ -500,15 +518,19 @@
TEST_AND_RETURN_FALSE(PayloadSigner::VerifySignature(signatures_message_data_,
key_path,
&signed_hash_data));
- const vector<char>& hash_data = hash_calculator_.raw_hash();
+ OmahaHashCalculator signed_hasher;
+ // TODO(petkov): Make sure signed_hash_context_ is loaded when resuming an
+ // update.
+ TEST_AND_RETURN_FALSE(signed_hasher.SetContext(signed_hash_context_));
+ TEST_AND_RETURN_FALSE(signed_hasher.Finalize());
+ const vector<char>& hash_data = signed_hasher.raw_hash();
TEST_AND_RETURN_FALSE(!hash_data.empty());
- return hash_data == signed_hash_data;
+ TEST_AND_RETURN_FALSE(hash_data == signed_hash_data);
+ return true;
}
-void DeltaPerformer::DiscardBufferHeadBytes(size_t count, bool do_hash) {
- if (do_hash) {
- hash_calculator_.Update(&buffer_[0], count);
- }
+void DeltaPerformer::DiscardBufferHeadBytes(size_t count) {
+ hash_calculator_.Update(&buffer_[0], count);
buffer_.erase(buffer_.begin(), buffer_.begin() + count);
}
@@ -532,11 +554,10 @@
&next_data_offset) &&
next_data_offset >= 0);
- string signed_sha256_context;
+ string sha256_context;
TEST_AND_RETURN_FALSE(
- prefs->GetString(kPrefsUpdateStateSignedSHA256Context,
- &signed_sha256_context) &&
- !signed_sha256_context.empty());
+ prefs->GetString(kPrefsUpdateStateSHA256Context, &sha256_context) &&
+ !sha256_context.empty());
int64_t manifest_metadata_size = 0;
TEST_AND_RETURN_FALSE(prefs->GetInt64(kPrefsManifestMetadataSize,
@@ -557,7 +578,7 @@
ResetUpdateProgress(prefs_);
if (last_updated_buffer_offset_ != buffer_offset_) {
TEST_AND_RETURN_FALSE(
- prefs_->SetString(kPrefsUpdateStateSignedSHA256Context,
+ prefs_->SetString(kPrefsUpdateStateSHA256Context,
hash_calculator_.GetContext()));
TEST_AND_RETURN_FALSE(prefs_->SetInt64(kPrefsUpdateStateNextDataOffset,
buffer_offset_));
diff --git a/delta_performer.h b/delta_performer.h
index 87a27dd..5425ea5 100644
--- a/delta_performer.h
+++ b/delta_performer.h
@@ -29,6 +29,7 @@
fd_(-1),
kernel_fd_(-1),
manifest_valid_(false),
+ manifest_metadata_size_(0),
next_operation_num_(0),
buffer_offset_(0),
last_updated_buffer_offset_(kuint64max),
@@ -51,12 +52,15 @@
int Close();
// Verifies the downloaded payload against the signed hash included in the
- // payload and returns true on success, false on failure. This method should
- // be called after closing the stream. Note this method returns true if the
- // public key is unavailable; it returns false if the public key is available
- // but the delta payload doesn't include a signature. If |public_key_path| is
- // an empty string, uses the default public key path.
- bool VerifyPayload(const std::string& public_key_path);
+ // payload as well as against the update check hash and size and returns true
+ // on success, false on failure. This method should be called after closing
+ // the stream. Note this method skips the signed hash check if the public key
+ // is unavailable; it returns false if the public key is available but the
+ // delta payload doesn't include a signature. If |public_key_path| is an empty
+ // string, uses the default public key path.
+ bool VerifyPayload(const std::string& public_key_path,
+ const std::string& update_check_response_hash,
+ const uint64_t update_check_response_size);
// Converts an ordered collection of Extent objects which contain data of
// length full_length to a comma-separated string. For each Extent, the
@@ -108,9 +112,9 @@
bool ExtractSignatureMessage(
const DeltaArchiveManifest_InstallOperation& operation);
- // Discard |count| bytes from the beginning of buffer_. If |do_hash| is true,
- // updates the hash calculator with these bytes before discarding them.
- void DiscardBufferHeadBytes(size_t count, bool do_hash);
+ // Updates the hash calculator with |count| bytes at the head of |buffer_| and
+ // then discards them.
+ void DiscardBufferHeadBytes(size_t count);
// Checkpoints the update progress into persistent storage to allow this
// update attempt to be resumed after reboot.
@@ -130,6 +134,7 @@
DeltaArchiveManifest manifest_;
bool manifest_valid_;
+ uint64_t manifest_metadata_size_;
// Index of the next operation to perform in the manifest.
int next_operation_num_;
@@ -148,9 +153,12 @@
// The block size (parsed from the manifest).
uint32_t block_size_;
- // Calculate the payload hash to verify against the signed hash.
+ // Calculates the payload hash.
OmahaHashCalculator hash_calculator_;
+ // Saves the signed hash context.
+ std::string signed_hash_context_;
+
// Signatures message blob extracted directly from the payload.
std::vector<char> signatures_message_data_;
diff --git a/delta_performer_unittest.cc b/delta_performer_unittest.cc
index 4a11c69..8717e07 100755
--- a/delta_performer_unittest.cc
+++ b/delta_performer_unittest.cc
@@ -251,8 +251,10 @@
.WillRepeatedly(Return(true));
EXPECT_CALL(prefs, SetInt64(kPrefsUpdateStateNextDataOffset, _))
.WillRepeatedly(Return(true));
- EXPECT_CALL(prefs, SetString(kPrefsUpdateStateSignedSHA256Context, _))
+ EXPECT_CALL(prefs, SetString(kPrefsUpdateStateSHA256Context, _))
.WillRepeatedly(Return(true));
+ EXPECT_CALL(prefs, SetString(kPrefsUpdateStateSignedSHA256Context, _))
+ .WillOnce(Return(true));
// Update the A image in place.
DeltaPerformer performer(&prefs);
@@ -278,7 +280,10 @@
strlen(new_data_string)));
EXPECT_TRUE(utils::FileExists(kUnittestPublicKeyPath));
- EXPECT_TRUE(performer.VerifyPayload(kUnittestPublicKeyPath));
+ EXPECT_TRUE(performer.VerifyPayload(
+ kUnittestPublicKeyPath,
+ OmahaHashCalculator::OmahaHashOfData(delta),
+ delta.size()));
}
TEST(DeltaPerformerTest, NewFullUpdateTest) {
@@ -287,7 +292,7 @@
const off_t kChunkSize = 128 * 1024;
FillWithData(&new_root);
FillWithData(&new_kern);
-
+
string new_root_path;
EXPECT_TRUE(utils::MakeTempFile("/tmp/NewFullUpdateTest_R.XXXXXX",
&new_root_path,
@@ -309,13 +314,13 @@
&out_blobs_fd));
ScopedPathUnlinker out_blobs_path_unlinker(out_blobs_path);
ScopedFdCloser out_blobs_fd_closer(&out_blobs_fd);
-
+
off_t out_blobs_length = 0;
-
+
Graph graph;
vector<DeltaArchiveManifest_InstallOperation> kernel_ops;
vector<Vertex::Index> final_order;
-
+
EXPECT_TRUE(DeltaDiffGenerator::ReadFullUpdateFromDisk(&graph,
new_kern_path,
new_root_path,
diff --git a/download_action.cc b/download_action.cc
index 4462ab8..b88e448 100644
--- a/download_action.cc
+++ b/download_action.cc
@@ -140,23 +140,28 @@
ActionExitCode code =
successful ? kActionCodeSuccess : kActionCodeDownloadTransferError;
if (code == kActionCodeSuccess) {
- // Makes sure the hash and size are correct.
- omaha_hash_calculator_.Finalize();
- if (omaha_hash_calculator_.hash() != install_plan_.download_hash) {
- LOG(ERROR) << "Download of " << install_plan_.download_url
- << " failed. Expected hash " << install_plan_.download_hash
- << " but got hash " << omaha_hash_calculator_.hash();
- code = kActionCodeDownloadHashMismatchError;
- } else if (bytes_received_ != install_plan_.size) {
- LOG(ERROR) << "Download of " << install_plan_.download_url
- << " failed. Expected size " << install_plan_.size
- << " but got size " << bytes_received_;
- code = kActionCodeDownloadSizeMismatchError;
- } else if (!install_plan_.is_full_update &&
- !delta_performer_->VerifyPayload("")) {
- LOG(ERROR) << "Download of " << install_plan_.download_url
- << " failed due to payload verification error.";
- code = kActionCodeDownloadPayloadVerificationError;
+ if (!install_plan_.is_full_update) {
+ if (!delta_performer_->VerifyPayload("",
+ install_plan_.download_hash,
+ install_plan_.size)) {
+ LOG(ERROR) << "Download of " << install_plan_.download_url
+ << " failed due to payload verification error.";
+ code = kActionCodeDownloadPayloadVerificationError;
+ }
+ } else {
+ // Makes sure the hash and size are correct for an old-style full update.
+ omaha_hash_calculator_.Finalize();
+ if (omaha_hash_calculator_.hash() != install_plan_.download_hash) {
+ LOG(ERROR) << "Download of " << install_plan_.download_url
+ << " failed. Expected hash " << install_plan_.download_hash
+ << " but got hash " << omaha_hash_calculator_.hash();
+ code = kActionCodeDownloadHashMismatchError;
+ } else if (bytes_received_ != install_plan_.size) {
+ LOG(ERROR) << "Download of " << install_plan_.download_url
+ << " failed. Expected size " << install_plan_.size
+ << " but got size " << bytes_received_;
+ code = kActionCodeDownloadSizeMismatchError;
+ }
}
}
diff --git a/prefs.cc b/prefs.cc
index 2de7eea..733443c 100644
--- a/prefs.cc
+++ b/prefs.cc
@@ -22,6 +22,7 @@
const char kPrefsUpdateCheckResponseHash[] = "update-check-response-hash";
const char kPrefsUpdateStateNextDataOffset[] = "update-state-next-data-offset";
const char kPrefsUpdateStateNextOperation[] = "update-state-next-operation";
+const char kPrefsUpdateStateSHA256Context[] = "update-state-sha-256-context";
const char kPrefsUpdateStateSignedSHA256Context[] =
"update-state-signed-sha-256-context";
diff --git a/prefs_interface.h b/prefs_interface.h
index a9b20a5..66fe8cb 100644
--- a/prefs_interface.h
+++ b/prefs_interface.h
@@ -16,6 +16,7 @@
extern const char kPrefsUpdateCheckResponseHash[];
extern const char kPrefsUpdateStateNextDataOffset[];
extern const char kPrefsUpdateStateNextOperation[];
+extern const char kPrefsUpdateStateSHA256Context[];
extern const char kPrefsUpdateStateSignedSHA256Context[];
// The prefs interface allows access to a persistent preferences