update_engine: Don't require hashes for non-official builds.
CL 228293 changed hash check logic such that hashes are always required
for official update URLs, on the assumption that they will always be
available.
This turns out to cause problems for situations like the referenced bug,
where the devserver counts as an official URL but isn't providing
hashes. This CL waives the hash check requirement for non-official
(dev/test) builds.
BUG=chromium:452139
TEST=cros_run_unit_tests --board=panther --packages=update_engine
Change-Id: I0a48bfe216a8afdff27e06baa24b3d37e7517b25
Reviewed-on: https://chromium-review.googlesource.com/243730
Trybot-Ready: David Pursell <dpursell@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Commit-Queue: David Pursell <dpursell@chromium.org>
Tested-by: David Pursell <dpursell@chromium.org>
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 8458db2..9f240fb 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -133,11 +133,21 @@
bool OmahaResponseHandlerAction::AreHashChecksMandatory(
const OmahaResponse& response) {
- // All our internal testing uses dev server which doesn't generate
- // metadata signatures by default, so we should waive hash checks for
- // unofficial URLs. dbus_service.cc does the security enforcement by not
- // allowing unofficial update URLs though except in specific cases.
- if (!system_state_->request_params()->IsUpdateUrlOfficial()) {
+ // We sometimes need to waive the hash checks in order to download from
+ // sources that don't provide hashes, such as dev server.
+ // At this point UpdateAttempter::IsAnyUpdateSourceAllowed() has already been
+ // checked, so an unofficial update URL won't get this far unless it's OK to
+ // use without a hash. Additionally, we want to always waive hash checks on
+ // unofficial builds (i.e. dev/test images).
+ // The end result is this:
+ // * Base image:
+ // - Official URLs require a hash.
+ // - Unofficial URLs only get this far if the IsAnyUpdateSourceAllowed()
+ // devmode/debugd checks pass, in which case the hash is waived.
+ // * Dev/test image:
+ // - Any URL is allowed through with no hash checking.
+ if (!system_state_->request_params()->IsUpdateUrlOfficial() ||
+ !system_state_->hardware()->IsOfficialBuild()) {
// Still do a hash check if a public key is included.
if (!response.public_key_rsa.empty()) {
// The autoupdate_CatchBadSignatures test checks for this string
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index 81a583a..bdbf3a7 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -242,6 +242,30 @@
EXPECT_EQ(in.version, install_plan.version);
}
+TEST_F(OmahaResponseHandlerActionTest,
+ HashChecksForOfficialUrlUnofficialBuildTest) {
+ // Official URLs for unofficial builds (dev/test images) don't require hash.
+ OmahaResponse in;
+ in.update_exists = true;
+ in.version = "a.b.c.d";
+ in.payload_urls.push_back("http://url.normally/needs/hash.checks.signed");
+ in.more_info_url = "http://more/info";
+ in.hash = "HASHj+";
+ in.size = 12;
+ FakeSystemState fake_system_state;
+ EXPECT_CALL(*(fake_system_state.mock_request_params()),
+ IsUpdateUrlOfficial())
+ .WillRepeatedly(Return(true));
+ fake_system_state.fake_hardware()->SetIsOfficialBuild(false);
+ InstallPlan install_plan;
+ EXPECT_TRUE(DoTestCommon(&fake_system_state, in, "/dev/sda5", "",
+ &install_plan));
+ EXPECT_EQ(in.payload_urls[0], install_plan.download_url);
+ EXPECT_EQ(in.hash, install_plan.payload_hash);
+ EXPECT_FALSE(install_plan.hash_checks_mandatory);
+ EXPECT_EQ(in.version, install_plan.version);
+}
+
TEST_F(OmahaResponseHandlerActionTest, HashChecksForHttpsTest) {
OmahaResponse in;
in.update_exists = true;
diff --git a/update_attempter.cc b/update_attempter.cc
index a6aa2b5..ae7f89b 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -1628,13 +1628,18 @@
}
bool UpdateAttempter::IsAnyUpdateSourceAllowed() {
- // Non-official (dev or test) builds can always use a custom update source.
+ // We allow updates from any source if either of these are true:
+ // * The device is running an unofficial (dev/test) image.
+ // * The debugd dev features are accessible (i.e. in devmode with no owner).
+ // This protects users running a base image, while still allowing a specific
+ // window (gated by the debug dev features) where `cros flash` is usable.
if (!system_state_->hardware()->IsOfficialBuild()) {
LOG(INFO) << "Non-official build; allowing any update source.";
return true;
}
- // Official images not in devmode are never allowed a custom update source.
+ // Even though the debugd tools are also gated on devmode, checking here can
+ // save us a D-Bus call so it's worth doing explicitly.
if (system_state_->hardware()->IsNormalBootMode()) {
LOG(INFO) << "Not in devmode; disallowing custom update sources.";
return false;