Add support for using public key sent by Omaha.
This adds support for Omaha to specify what RSA public to use for
verifying both the metadata hash signature and the payload itself.
For security reasons, we only allow this for non-official builds
e.g. for official builds we keep using the key stored on the root
file-system.
Also, if the key is specified in the Omaha response then we make hash
checks mandatory; e.g. if the signatures don't check out, fail the
update.
See CL:175283 for the devserver changes to transmit the public key and
signed metadata hash.
BUG=chromium:264352
TEST=New unit tests + unit tests pass + manual testing.
Change-Id: I709be02662a484c6284bb78683b973554e482928
Reviewed-on: https://chromium-review.googlesource.com/175285
Reviewed-by: Don Garrett <dgarrett@chromium.org>
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>
diff --git a/delta_performer.cc b/delta_performer.cc
index 69ffbc1..15f9002 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -12,6 +12,7 @@
#include <string>
#include <vector>
+#include <base/file_util.h>
#include <base/memory/scoped_ptr.h>
#include <base/string_util.h>
#include <base/stringprintf.h>
@@ -23,6 +24,7 @@
#include "update_engine/extent_ranges.h"
#include "update_engine/extent_writer.h"
#include "update_engine/graph_types.h"
+#include "update_engine/hardware_interface.h"
#include "update_engine/payload_signer.h"
#include "update_engine/payload_state_interface.h"
#include "update_engine/prefs_interface.h"
@@ -801,6 +803,19 @@
return true;
}
+bool DeltaPerformer::GetPublicKeyFromResponse(base::FilePath *out_tmp_key) {
+ if (system_state_->hardware()->IsOfficialBuild() ||
+ utils::FileExists(public_key_path_.c_str()) ||
+ install_plan_->public_key_rsa.empty())
+ return false;
+
+ if (!utils::DecodeAndStoreBase64String(install_plan_->public_key_rsa,
+ out_tmp_key))
+ return false;
+
+ return true;
+}
+
ErrorCode DeltaPerformer::ValidateMetadataSignature(
const char* metadata, uint64_t metadata_size) {
@@ -825,9 +840,21 @@
return kErrorCodeDownloadMetadataSignatureError;
}
+ // 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;
+ if (GetPublicKeyFromResponse(&tmp_key))
+ path_to_public_key = tmp_key;
+ ScopedPathUnlinker tmp_key_remover(tmp_key.value());
+ if (tmp_key.empty())
+ tmp_key_remover.set_should_remove(false);
+
+ LOG(INFO) << "Verifying metadata hash signature using public key: "
+ << path_to_public_key.value();
+
vector<char> expected_metadata_hash;
if (!PayloadSigner::GetRawHashFromSignature(metadata_signature,
- public_key_path_,
+ path_to_public_key.value(),
&expected_metadata_hash)) {
LOG(ERROR) << "Unable to compute expected hash from metadata signature";
return kErrorCodeDownloadMetadataSignatureError;
@@ -855,7 +882,7 @@
return kErrorCodeDownloadMetadataSignatureMismatch;
}
- LOG(INFO) << "Manifest signature matches expected value in Omaha response";
+ LOG(INFO) << "Metadata hash signature matches value in Omaha response.";
return kErrorCodeSuccess;
}
@@ -961,7 +988,18 @@
ErrorCode DeltaPerformer::VerifyPayload(
const std::string& update_check_response_hash,
const uint64_t update_check_response_size) {
- LOG(INFO) << "Verifying delta payload using public key: " << public_key_path_;
+
+ // 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;
+ if (GetPublicKeyFromResponse(&tmp_key))
+ path_to_public_key = tmp_key;
+ ScopedPathUnlinker tmp_key_remover(tmp_key.value());
+ if (tmp_key.empty())
+ tmp_key_remover.set_should_remove(false);
+
+ LOG(INFO) << "Verifying payload using public key: "
+ << path_to_public_key.value();
// Verifies the download size.
TEST_AND_RETURN_VAL(kErrorCodePayloadSizeMismatchError,
@@ -976,7 +1014,7 @@
payload_hash_data == update_check_response_hash);
// Verifies the signed payload hash.
- if (!utils::FileExists(public_key_path_.c_str())) {
+ if (!utils::FileExists(path_to_public_key.value().c_str())) {
LOG(WARNING) << "Not verifying signed delta payload -- missing public key.";
return kErrorCodeSuccess;
}
@@ -986,7 +1024,7 @@
TEST_AND_RETURN_VAL(kErrorCodeDownloadPayloadPubKeyVerificationError,
PayloadSigner::VerifySignature(
signatures_message_data_,
- public_key_path_,
+ path_to_public_key.value(),
&signed_hash_data));
OmahaHashCalculator signed_hasher;
TEST_AND_RETURN_VAL(kErrorCodeDownloadPayloadPubKeyVerificationError,
@@ -1006,6 +1044,8 @@
return kErrorCodeDownloadPayloadPubKeyVerificationError;
}
+ LOG(INFO) << "Payload hash matches value in payload.";
+
// At this point, we are guaranteed to have downloaded a full payload, i.e
// the one whose size matches the size mentioned in Omaha response. If any
// errors happen after this, it's likely a problem with the payload itself or
diff --git a/delta_performer.h b/delta_performer.h
index a9cbe44..ca694ed 100644
--- a/delta_performer.h
+++ b/delta_performer.h
@@ -179,6 +179,7 @@
private:
friend class DeltaPerformerTest;
FRIEND_TEST(DeltaPerformerTest, IsIdempotentOperationTest);
+ FRIEND_TEST(DeltaPerformerTest, UsePublicKeyFromResponse);
// Logs the progress of downloading/applying an update.
void LogProgress(const char* message_prefix);
@@ -257,6 +258,14 @@
// Sends UMA statistics for the given error code.
void SendUmaStat(ErrorCode code);
+ // If the Omaha response contains a public RSA key and we're allowed
+ // to use it (e.g. if we're in developer mode), extract the key from
+ // the response and store it in a temporary file and return true. In
+ // the affirmative the path to the temporary file is stored in
+ // |out_tmp_key| and it is the responsibility of the caller to clean
+ // it up.
+ bool GetPublicKeyFromResponse(base::FilePath *out_tmp_key);
+
// Update Engine preference store.
PrefsInterface* prefs_;
diff --git a/delta_performer_unittest.cc b/delta_performer_unittest.cc
index 7836335..80aa064 100644
--- a/delta_performer_unittest.cc
+++ b/delta_performer_unittest.cc
@@ -20,6 +20,7 @@
#include "update_engine/delta_diff_generator.h"
#include "update_engine/delta_performer.h"
#include "update_engine/extent_ranges.h"
+#include "update_engine/fake_hardware.h"
#include "update_engine/full_update_generator.h"
#include "update_engine/graph_types.h"
#include "update_engine/mock_system_state.h"
@@ -1140,4 +1141,83 @@
DoOperationHashMismatchTest(kInvalidOperationData, true);
}
+TEST(DeltaPerformerTest, UsePublicKeyFromResponse) {
+ PrefsMock prefs;
+ MockSystemState mock_system_state;
+ InstallPlan install_plan;
+ base::FilePath key_path;
+
+ // The result of the GetPublicKeyResponse() method is based on three things
+ //
+ // 1. Whether it's an official build; and
+ // 2. Whether the Public RSA key to be used is in the root filesystem; and
+ // 3. Whether the reponse has a public key
+ //
+ // We test all eight combinations to ensure that we only use the
+ // public key in the response if
+ //
+ // a. it's not an official build; and
+ // b. there is no key in the root filesystem.
+
+ DeltaPerformer *performer = new DeltaPerformer(&prefs,
+ &mock_system_state,
+ &install_plan);
+ FakeHardware* fake_hardware = mock_system_state.get_fake_hardware();
+
+ string temp_dir;
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PublicKeyFromResponseTests.XXXXXX",
+ &temp_dir));
+ string non_existing_file = temp_dir + "/non-existing";
+ string existing_file = temp_dir + "/existing";
+ EXPECT_EQ(0, System(StringPrintf("touch %s", existing_file.c_str())));
+
+ // Non-official build, non-existing public-key, key in response -> true
+ fake_hardware->SetIsOfficialBuild(false);
+ performer->public_key_path_ = non_existing_file;
+ install_plan.public_key_rsa = "VGVzdAo="; // result of 'echo "Test" | base64'
+ EXPECT_TRUE(performer->GetPublicKeyFromResponse(&key_path));
+ EXPECT_FALSE(key_path.empty());
+ EXPECT_EQ(unlink(key_path.value().c_str()), 0);
+ // Same with official build -> false
+ fake_hardware->SetIsOfficialBuild(true);
+ EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
+
+ // Non-official build, existing public-key, key in response -> false
+ fake_hardware->SetIsOfficialBuild(false);
+ performer->public_key_path_ = existing_file;
+ install_plan.public_key_rsa = "VGVzdAo="; // result of 'echo "Test" | base64'
+ EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
+ // Same with official build -> false
+ fake_hardware->SetIsOfficialBuild(true);
+ EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
+
+ // Non-official build, non-existing public-key, no key in response -> false
+ fake_hardware->SetIsOfficialBuild(false);
+ performer->public_key_path_ = non_existing_file;
+ install_plan.public_key_rsa = "";
+ EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
+ // Same with official build -> false
+ fake_hardware->SetIsOfficialBuild(true);
+ EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
+
+ // Non-official build, existing public-key, no key in response -> false
+ fake_hardware->SetIsOfficialBuild(false);
+ performer->public_key_path_ = existing_file;
+ install_plan.public_key_rsa = "";
+ EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
+ // Same with official build -> false
+ fake_hardware->SetIsOfficialBuild(true);
+ EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
+
+ // Non-official build, non-existing public-key, key in response
+ // but invalid base64 -> false
+ fake_hardware->SetIsOfficialBuild(false);
+ performer->public_key_path_ = non_existing_file;
+ install_plan.public_key_rsa = "not-valid-base64";
+ EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
+
+ delete performer;
+ EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
+}
+
} // namespace chromeos_update_engine
diff --git a/download_action_unittest.cc b/download_action_unittest.cc
index be9d170..7bb0fc5 100644
--- a/download_action_unittest.cc
+++ b/download_action_unittest.cc
@@ -149,6 +149,7 @@
0,
"",
output_temp_file.GetPath(),
+ "",
"");
ObjectFeederAction<InstallPlan> feeder_action;
feeder_action.set_obj(install_plan);
@@ -265,7 +266,7 @@
// takes ownership of passed in HttpFetcher
ObjectFeederAction<InstallPlan> feeder_action;
InstallPlan install_plan(false, false, "", 0, "", 0, "",
- temp_file.GetPath(), "");
+ temp_file.GetPath(), "", "");
feeder_action.set_obj(install_plan);
PrefsMock prefs;
DownloadAction download_action(&prefs, NULL,
@@ -374,7 +375,8 @@
0,
"",
"/dev/null",
- "/dev/null");
+ "/dev/null",
+ "");
ObjectFeederAction<InstallPlan> feeder_action;
feeder_action.set_obj(install_plan);
PrefsMock prefs;
@@ -409,7 +411,7 @@
DirectFileWriter writer;
// takes ownership of passed in HttpFetcher
- InstallPlan install_plan(false, false, "", 0, "", 0, "", path, "");
+ InstallPlan install_plan(false, false, "", 0, "", 0, "", path, "", "");
ObjectFeederAction<InstallPlan> feeder_action;
feeder_action.set_obj(install_plan);
PrefsMock prefs;
@@ -486,6 +488,7 @@
0,
"",
output_temp_file.GetPath(),
+ "",
"");
ObjectFeederAction<InstallPlan> feeder_action;
feeder_action.set_obj(install_plan);
diff --git a/filesystem_copier_action_unittest.cc b/filesystem_copier_action_unittest.cc
index 685c546..7573c91 100644
--- a/filesystem_copier_action_unittest.cc
+++ b/filesystem_copier_action_unittest.cc
@@ -315,7 +315,7 @@
ObjectFeederAction<InstallPlan> feeder_action;
const char* kUrl = "http://some/url";
- InstallPlan install_plan(false, true, kUrl, 0, "", 0, "", "", "");
+ InstallPlan install_plan(false, true, kUrl, 0, "", 0, "", "", "", "");
feeder_action.set_obj(install_plan);
FilesystemCopierAction copier_action(&mock_system_state_, false, false);
ObjectCollectorAction<InstallPlan> collector_action;
@@ -348,7 +348,8 @@
0,
"",
"/no/such/file",
- "/no/such/file");
+ "/no/such/file",
+ "");
feeder_action.set_obj(install_plan);
FilesystemCopierAction copier_action(&mock_system_state_, false, false);
ObjectCollectorAction<InstallPlan> collector_action;
diff --git a/install_plan.cc b/install_plan.cc
index a17cb52..1749cbe 100644
--- a/install_plan.cc
+++ b/install_plan.cc
@@ -20,7 +20,8 @@
uint64_t metadata_size,
const string& metadata_signature,
const string& install_path,
- const string& kernel_install_path)
+ const string& kernel_install_path,
+ const std::string& public_key_rsa)
: is_resume(is_resume),
is_full_update(is_full_update),
download_url(url),
@@ -33,7 +34,8 @@
kernel_size(0),
rootfs_size(0),
hash_checks_mandatory(false),
- powerwash_required(false) {}
+ powerwash_required(false),
+ public_key_rsa(public_key_rsa) {}
InstallPlan::InstallPlan() : is_resume(false),
is_full_update(false), // play it safe.
diff --git a/install_plan.h b/install_plan.h
index 3ec9d55..29ce212 100644
--- a/install_plan.h
+++ b/install_plan.h
@@ -25,7 +25,8 @@
uint64_t metadata_size,
const std::string& metadata_signature,
const std::string& install_path,
- const std::string& kernel_install_path);
+ const std::string& kernel_install_path,
+ const std::string& public_key_rsa);
// Default constructor: Initialize all members which don't have a class
// initializer.
@@ -71,6 +72,10 @@
// True if Powerwash is required on reboot after applying the payload.
// False otherwise.
bool powerwash_required;
+
+ // If not blank, a base-64 encoded representation of the PEM-encoded
+ // public key in the response.
+ std::string public_key_rsa;
};
class InstallPlanAction;
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 5ce59bb..32b6530 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -53,6 +53,7 @@
static const char* kTagSha256 = "sha256";
static const char* kTagDisableP2PForDownloading = "DisableP2PForDownloading";
static const char* kTagDisableP2PForSharing = "DisableP2PForSharing";
+static const char* kTagPublicKeyRsa = "PublicKeyRsa";
namespace {
@@ -699,6 +700,8 @@
(XmlGetProperty(pie_action_node, kTagDisableP2PForDownloading) == "true");
output_object->disable_p2p_for_sharing =
(XmlGetProperty(pie_action_node, kTagDisableP2PForSharing) == "true");
+ output_object->public_key_rsa =
+ XmlGetProperty(pie_action_node, kTagPublicKeyRsa);
string max = XmlGetProperty(pie_action_node, kTagMaxFailureCountPerUrl);
if (!base::StringToUint(max, &output_object->max_failure_count_per_url))
diff --git a/omaha_response.h b/omaha_response.h
index 04fb878..6580077 100644
--- a/omaha_response.h
+++ b/omaha_response.h
@@ -69,6 +69,10 @@
// True if the Omaha rule instructs us to disable p2p for sharing.
bool disable_p2p_for_sharing;
+
+ // If not blank, a base-64 encoded representation of the PEM-encoded
+ // public key in the response.
+ std::string public_key_rsa;
};
COMPILE_ASSERT(sizeof(off_t) == 8, off_t_not_64bit);
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 4625037..c145ae6 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -88,6 +88,7 @@
install_plan_.payload_hash = response.hash;
install_plan_.metadata_size = response.metadata_size;
install_plan_.metadata_signature = response.metadata_signature;
+ install_plan_.public_key_rsa = response.public_key_rsa;
install_plan_.hash_checks_mandatory = AreHashChecksMandatory(response);
install_plan_.is_resume =
DeltaPerformer::CanResumeUpdate(system_state_->prefs(), response.hash);
@@ -138,17 +139,27 @@
bool OmahaResponseHandlerAction::AreHashChecksMandatory(
const OmahaResponse& response) {
- // All our internal testing uses dev server which doesn't generate metadata
- // signatures yet. So, in order not to break image_to_live or other AU tools,
- // we should waive the hash checks for those cases. Since all internal
- // testing is done using a dev_image or test_image, we can use that as a
- // criteria for waiving. This criteria reduces the attack surface as
- // opposed to waiving the checks when we're in dev mode, because we do want
- // to enforce the hash checks when our end customers run in dev mode if they
- // are using an official build, so that they are protected more.
+ // All our internal testing uses dev server which doesn't generate
+ // metadata signatures by default. So, in order not to break
+ // image_to_live or other AU tools, we should waive the hash checks
+ // for those cases, except if the response indicates that the
+ // payload is signed.
+ //
+ // Since all internal testing is done using a dev_image or
+ // test_image, we can use that as a criteria for waiving. This
+ // criteria reduces the attack surface as opposed to waiving the
+ // checks when we're in dev mode, because we do want to enforce the
+ // hash checks when our end customers run in dev mode if they are
+ // using an official build, so that they are protected more.
if (!system_state_->hardware()->IsOfficialBuild()) {
- LOG(INFO) << "Waiving payload hash checks for unofficial builds";
- return false;
+ if (!response.public_key_rsa.empty()) {
+ LOG(INFO) << "Mandating payload hash checks since Omaha Response "
+ << "for unofficial build includes public RSA key.";
+ return true;
+ } else {
+ LOG(INFO) << "Waiving payload hash checks for unofficial builds";
+ return false;
+ }
}
// If we're using p2p, |install_plan_.download_url| may contain a
diff --git a/utils.cc b/utils.cc
index 47f8d78..7e5cf75 100644
--- a/utils.cc
+++ b/utils.cc
@@ -1070,6 +1070,48 @@
return xattr_res == 0;
}
+bool DecodeAndStoreBase64String(const std::string& base64_encoded,
+ base::FilePath *out_path) {
+ vector<char> contents;
+
+ out_path->clear();
+
+ if (base64_encoded.size() == 0) {
+ LOG(ERROR) << "Can't decode empty string.";
+ return false;
+ }
+
+ if (!OmahaHashCalculator::Base64Decode(base64_encoded, &contents) ||
+ contents.size() == 0) {
+ LOG(ERROR) << "Error decoding base64.";
+ return false;
+ }
+
+ FILE *file = file_util::CreateAndOpenTemporaryFile(out_path);
+ if (file == NULL) {
+ LOG(ERROR) << "Error creating temporary file.";
+ return false;
+ }
+
+ if (fwrite(&contents[0], 1, contents.size(), file) != contents.size()) {
+ PLOG(ERROR) << "Error writing to temporary file.";
+ if (fclose(file) != 0)
+ PLOG(ERROR) << "Error closing temporary file.";
+ if (unlink(out_path->value().c_str()) != 0)
+ PLOG(ERROR) << "Error unlinking temporary file.";
+ out_path->clear();
+ return false;
+ }
+
+ if (fclose(file) != 0) {
+ PLOG(ERROR) << "Error closing temporary file.";
+ out_path->clear();
+ return false;
+ }
+
+ return true;
+}
+
} // namespace utils
} // namespace chromeos_update_engine
diff --git a/utils.h b/utils.h
index ee99d86..710c5db 100644
--- a/utils.h
+++ b/utils.h
@@ -336,6 +336,14 @@
// supported, false if not or if an error occured.
bool IsXAttrSupported(const base::FilePath& dir_path);
+// Decodes the data in |base64_encoded| and stores it in a temporary
+// file. Returns false if the given data is empty, not well-formed
+// base64 or if an error occurred. If true is returned, the decoded
+// data is stored in the file returned in |out_path|. The file should
+// be deleted when no longer needed.
+bool DecodeAndStoreBase64String(const std::string& base64_encoded,
+ base::FilePath *out_path);
+
} // namespace utils
diff --git a/utils_unittest.cc b/utils_unittest.cc
index 1676444..c1f9a2e 100644
--- a/utils_unittest.cc
+++ b/utils_unittest.cc
@@ -388,4 +388,26 @@
utils::TimeFromStructTimespec(&ts));
}
+TEST(UtilsTest, DecodeAndStoreBase64String) {
+ base::FilePath path;
+
+ // Ensure we return false on empty strings or invalid base64.
+ EXPECT_FALSE(utils::DecodeAndStoreBase64String("", &path));
+ EXPECT_FALSE(utils::DecodeAndStoreBase64String("not valid base64", &path));
+
+ // Pass known base64 and check that it matches. This string was generated
+ // the following way:
+ //
+ // $ echo "Update Engine" | base64
+ // VXBkYXRlIEVuZ2luZQo=
+ EXPECT_TRUE(utils::DecodeAndStoreBase64String("VXBkYXRlIEVuZ2luZQo=",
+ &path));
+ ScopedPathUnlinker unlinker(path.value());
+ string expected_contents = "Update Engine\n";
+ string contents;
+ EXPECT_TRUE(utils::ReadFile(path.value(), &contents));
+ EXPECT_EQ(contents, expected_contents);
+ EXPECT_EQ(utils::FileSize(path.value()), expected_contents.size());
+}
+
} // namespace chromeos_update_engine