Make public key verification check binding.
Until now, we've just warned on failure. This CL makes the update fail
if the check fails.
BUG=chromium-os:19872
TEST=unittests; tested on device
Change-Id: I485b2548849f46d2b802c478736671bb44a85aab
Reviewed-on: http://gerrit.chromium.org/gerrit/6998
Reviewed-by: Darin Petkov <petkov@chromium.org>
Tested-by: Andrew de los Reyes <adlr@chromium.org>
diff --git a/delta_performer.cc b/delta_performer.cc
index 25ea99a..fa35ce8 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -583,22 +583,19 @@
return true;
}
-#define TEST_SET_TRUE_RET_TRUE(_ptr, _condition) \
- do { \
- if (!(_condition)) { \
- LOG(ERROR) << "Non fatal public key verification: " << #_condition; \
- if (_ptr) { \
- *(_ptr) = true; \
- } \
- return true; \
- } \
- } while(0)
+#define TEST_AND_RETURN_VAL(_retval, _condition) \
+ do { \
+ if (!(_condition)) { \
+ LOG(ERROR) << "VerifyPayload failure: " << #_condition; \
+ return _retval; \
+ } \
+ } while (0);
-bool DeltaPerformer::VerifyPayload(
+
+ActionExitCode DeltaPerformer::VerifyPayload(
const string& public_key_path,
const std::string& update_check_response_hash,
- const uint64_t update_check_response_size,
- bool* signature_failed) {
+ const uint64_t update_check_response_size) {
string key_path = public_key_path;
if (key_path.empty()) {
key_path = kUpdatePayloadPublicKeyPath;
@@ -607,47 +604,49 @@
// 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);
+ TEST_AND_RETURN_VAL(kActionCodeDownloadPayloadVerificationError,
+ !download_hash_data.empty());
+ TEST_AND_RETURN_VAL(kActionCodeDownloadPayloadVerificationError,
+ download_hash_data == update_check_response_hash);
// Verifies the download size.
- TEST_AND_RETURN_FALSE(update_check_response_size ==
- manifest_metadata_size_ + buffer_offset_);
+ TEST_AND_RETURN_VAL(kActionCodeDownloadPayloadVerificationError,
+ 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 signed delta payload -- missing public key.";
- return true;
+ return kActionCodeSuccess;
}
- TEST_SET_TRUE_RET_TRUE(signature_failed, !signatures_message_data_.empty());
+ TEST_AND_RETURN_VAL(kActionCodeSignedDeltaPayloadExpectedError,
+ !signatures_message_data_.empty());
vector<char> signed_hash_data;
- TEST_SET_TRUE_RET_TRUE(signature_failed, PayloadSigner::VerifySignature(
- signatures_message_data_,
- key_path,
- &signed_hash_data));
+ TEST_AND_RETURN_VAL(kActionCodeDownloadPayloadPubKeyVerificationError,
+ PayloadSigner::VerifySignature(
+ signatures_message_data_,
+ key_path,
+ &signed_hash_data));
OmahaHashCalculator signed_hasher;
- TEST_SET_TRUE_RET_TRUE(signature_failed,
- signed_hasher.SetContext(signed_hash_context_));
- TEST_SET_TRUE_RET_TRUE(signature_failed,
- signed_hasher.Finalize());
+ TEST_AND_RETURN_VAL(kActionCodeDownloadPayloadPubKeyVerificationError,
+ signed_hasher.SetContext(signed_hash_context_));
+ TEST_AND_RETURN_VAL(kActionCodeDownloadPayloadPubKeyVerificationError,
+ signed_hasher.Finalize());
vector<char> hash_data = signed_hasher.raw_hash();
PayloadSigner::PadRSA2048SHA256Hash(&hash_data);
- TEST_SET_TRUE_RET_TRUE(signature_failed, !hash_data.empty());
+ TEST_AND_RETURN_VAL(kActionCodeDownloadPayloadPubKeyVerificationError,
+ !hash_data.empty());
if (hash_data != signed_hash_data) {
- LOG(ERROR) << "Public key verificaion failed. This is non-fatal. "
+ LOG(ERROR) << "Public key verification failed, thus update failed. "
"Attached Signature:";
utils::HexDumpVector(signed_hash_data);
LOG(ERROR) << "Computed Signature:";
utils::HexDumpVector(hash_data);
- if (signature_failed) {
- *signature_failed = true;
- }
+ return kActionCodeDownloadPayloadPubKeyVerificationError;
}
- return true;
+ return kActionCodeSuccess;
}
-#undef TEST_SET_TRUE_RET_TRUE
-
bool DeltaPerformer::GetNewPartitionInfo(uint64_t* kernel_size,
vector<char>* kernel_hash,
uint64_t* rootfs_size,
diff --git a/delta_performer.h b/delta_performer.h
index a80e180..5c1f3ab 100644
--- a/delta_performer.h
+++ b/delta_performer.h
@@ -61,18 +61,16 @@
int Close();
// Verifies the downloaded payload against the signed hash included in the
- // 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.
- // If the signature check fails, *signature_failed will be set to true (if
- // it's non-NULL); this will not cause the method to fail.
- bool VerifyPayload(const std::string& public_key_path,
- const std::string& update_check_response_hash,
- const uint64_t update_check_response_size,
- bool* signature_failed);
+ // payload, against the update check hash and size, and against the public
+ // key and returns kActionCodeSuccess on success, an error code 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
+ // kActionCodeSignedDeltaPayloadExpectedError 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.
+ ActionExitCode VerifyPayload(const std::string& public_key_path,
+ const std::string& update_check_response_hash,
+ const uint64_t update_check_response_size);
// Reads from the update manifest the expected sizes and hashes of the target
// kernel and rootfs partitions. These values can be used for applied update
diff --git a/delta_performer_unittest.cc b/delta_performer_unittest.cc
index 3bdb2d8..6318806 100644
--- a/delta_performer_unittest.cc
+++ b/delta_performer_unittest.cc
@@ -452,21 +452,24 @@
strlen(new_data_string)));
EXPECT_TRUE(utils::FileExists(kUnittestPublicKeyPath));
- const bool expect_public_verify_failure =
- signature_test == kSignatureNone ||
- signature_test == kSignatureGeneratedShellBadKey;
- bool public_verify_failure = false;
- EXPECT_TRUE(performer.VerifyPayload(
+ ActionExitCode expect_verify_result = kActionCodeSuccess;
+ switch (signature_test) {
+ case kSignatureNone:
+ expect_verify_result = kActionCodeSignedDeltaPayloadExpectedError;
+ break;
+ case kSignatureGeneratedShellBadKey:
+ expect_verify_result = kActionCodeDownloadPayloadPubKeyVerificationError;
+ break;
+ default: break; // appease gcc
+ }
+ EXPECT_EQ(expect_verify_result, performer.VerifyPayload(
kUnittestPublicKeyPath,
OmahaHashCalculator::OmahaHashOfData(delta),
- delta.size(),
- &public_verify_failure));
- EXPECT_EQ(expect_public_verify_failure, public_verify_failure);
- EXPECT_TRUE(performer.VerifyPayload(
+ delta.size()));
+ EXPECT_EQ(kActionCodeSuccess, performer.VerifyPayload(
"/public/key/does/not/exists",
OmahaHashCalculator::OmahaHashOfData(delta),
- delta.size(),
- NULL));
+ delta.size()));
uint64_t new_kernel_size;
vector<char> new_kernel_hash;
diff --git a/download_action.cc b/download_action.cc
index 1491caa..9da925c 100644
--- a/download_action.cc
+++ b/download_action.cc
@@ -27,8 +27,7 @@
http_fetcher_(http_fetcher),
code_(kActionCodeSuccess),
delegate_(NULL),
- bytes_received_(0),
- skip_reporting_signature_fail_(NULL) {}
+ bytes_received_(0) {}
DownloadAction::~DownloadAction() {}
@@ -158,18 +157,16 @@
if (delegate_) {
delegate_->SetDownloadStatus(false); // Set to inactive.
}
- bool signature_verify_failed = false;
ActionExitCode code =
successful ? kActionCodeSuccess : kActionCodeDownloadTransferError;
if (code == kActionCodeSuccess) {
if (!install_plan_.is_full_update) {
- if (!delta_performer_->VerifyPayload("",
- install_plan_.download_hash,
- install_plan_.size,
- &signature_verify_failed)) {
+ code = delta_performer_->VerifyPayload("",
+ install_plan_.download_hash,
+ install_plan_.size);
+ if (code != kActionCodeSuccess) {
LOG(ERROR) << "Download of " << install_plan_.download_url
<< " failed due to payload verification error.";
- code = kActionCodeDownloadPayloadVerificationError;
} else if (!delta_performer_->GetNewPartitionInfo(
&install_plan_.kernel_size,
&install_plan_.kernel_hash,
@@ -195,12 +192,6 @@
}
}
- if (skip_reporting_signature_fail_.get() &&
- (code != kActionCodeSuccess || !signature_verify_failed)) {
- LOG(INFO) << "Suppressing signature pub key verification warning";
- skip_reporting_signature_fail_->Run();
- }
-
FlushLinuxCaches();
// Write the path to the output pipe if we're successful.
diff --git a/download_action.h b/download_action.h
index dcdc6fb..373e6fe 100644
--- a/download_action.h
+++ b/download_action.h
@@ -98,10 +98,6 @@
HttpFetcher* http_fetcher() { return http_fetcher_.get(); }
- void set_skip_reporting_signature_fail(google::protobuf::Closure* callback) {
- skip_reporting_signature_fail_.reset(callback);
- }
-
private:
// The InstallPlan passed in
InstallPlan install_plan_;
@@ -138,9 +134,6 @@
DownloadActionDelegate* delegate_;
uint64_t bytes_received_;
- // Called if the download fails OR (download success AND signature verifies)
- scoped_ptr<google::protobuf::Closure> skip_reporting_signature_fail_;
-
DISALLOW_COPY_AND_ASSIGN(DownloadAction);
};
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 78af1c0..8080b64 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -182,8 +182,7 @@
http_fetcher_(http_fetcher),
ping_only_(ping_only),
ping_active_days_(0),
- ping_roll_call_days_(0),
- should_skip_(false) {}
+ ping_roll_call_days_(0) {}
OmahaRequestAction::~OmahaRequestAction() {}
@@ -220,10 +219,6 @@
}
void OmahaRequestAction::PerformAction() {
- if (should_skip_) {
- processor_->ActionComplete(this, kActionCodeSuccess);
- return;
- }
http_fetcher_->set_delegate(this);
InitPingDays();
if (ping_only_ &&
diff --git a/omaha_request_action.h b/omaha_request_action.h
index 753ab57..2edc13c 100644
--- a/omaha_request_action.h
+++ b/omaha_request_action.h
@@ -151,8 +151,6 @@
// Returns true if this is an Event request, false if it's an UpdateCheck.
bool IsEvent() const { return event_.get() != NULL; }
- void set_should_skip(bool should_skip) { should_skip_ = should_skip; }
-
private:
// If this is an update check request, initializes
// |ping_active_days_| and |ping_roll_call_days_| to values that may
@@ -187,9 +185,6 @@
int ping_active_days_;
int ping_roll_call_days_;
- // If true, this action should be a noop.
- bool should_skip_;
-
DISALLOW_COPY_AND_ASSIGN(OmahaRequestAction);
};
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index 7a0d796..6f4df63 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -288,33 +288,6 @@
EXPECT_FALSE(processor.IsRunning());
}
-TEST(OmahaRequestActionTest, SkipTest) {
- const string http_response("invalid xml>");
-
- GMainLoop *loop = g_main_loop_new(g_main_context_default(), FALSE);
-
- NiceMock<PrefsMock> prefs;
- MockHttpFetcher* fetcher = new MockHttpFetcher(http_response.data(),
- http_response.size(),
- NULL);
- fetcher->set_never_use(true);
- OmahaRequestAction action(&prefs, kDefaultTestParams,
- new OmahaEvent(OmahaEvent::kTypeUpdateComplete),
- fetcher, // Passes fetcher ownership
- false);
- action.set_should_skip(true);
- OmahaRequestActionTestProcessorDelegate delegate;
- delegate.loop_ = loop;
- ActionProcessor processor;
- processor.set_delegate(&delegate);
- processor.EnqueueAction(&action);
-
- g_timeout_add(0, &StartProcessorInRunLoop, &processor);
- g_main_loop_run(loop);
- g_main_loop_unref(loop);
- EXPECT_FALSE(processor.IsRunning());
-}
-
TEST(OmahaRequestActionTest, InvalidXmlTest) {
OmahaResponse response;
ASSERT_FALSE(
diff --git a/update_attempter.cc b/update_attempter.cc
index 27d3a91..57ddc66 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -217,22 +217,6 @@
shared_ptr<DownloadAction> download_action(
new DownloadAction(prefs_, new MultiRangeHTTPFetcher(
new LibcurlHttpFetcher(GetProxyResolver()))));
- // This action is always initially in place to warn OS vendor of a
- // signature failure. If it's not needed, it will be told to skip.
- shared_ptr<OmahaRequestAction> download_signature_warning(
- new OmahaRequestAction(
- prefs_,
- omaha_request_params_,
- new OmahaEvent(
- OmahaEvent::kTypeUpdateDownloadFinished,
- OmahaEvent::kResultError,
- kActionCodeDownloadPayloadPubKeyVerificationError),
- new LibcurlHttpFetcher(GetProxyResolver()),
- false));
- download_action->set_skip_reporting_signature_fail(
- NewPermanentCallback(download_signature_warning.get(),
- &OmahaRequestAction::set_should_skip,
- true));
shared_ptr<OmahaRequestAction> download_finished_action(
new OmahaRequestAction(prefs_,
omaha_request_params_,
@@ -264,7 +248,6 @@
kernel_filesystem_copier_action));
actions_.push_back(shared_ptr<AbstractAction>(download_started_action));
actions_.push_back(shared_ptr<AbstractAction>(download_action));
- actions_.push_back(shared_ptr<AbstractAction>(download_signature_warning));
actions_.push_back(shared_ptr<AbstractAction>(download_finished_action));
actions_.push_back(shared_ptr<AbstractAction>(filesystem_verifier_action));
actions_.push_back(shared_ptr<AbstractAction>(
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 673471c..b0dccf8 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -283,7 +283,6 @@
OmahaRequestAction::StaticType(),
DownloadAction::StaticType(),
OmahaRequestAction::StaticType(),
- OmahaRequestAction::StaticType(),
FilesystemCopierAction::StaticType(),
FilesystemCopierAction::StaticType(),
PostinstallRunnerAction::StaticType(),