applypatch: Restrict applypatch_check to eMMC targets.

Also fix an error-pone behavior in previous code when verifying an eMMC
target. As long as it loads the partition content successfully according
to the SHAs embedded in the filename, it shouldn't further check against
the SHAs given in the second argument. Because the loaded contents
relate to a specific partition size.

For example:

  apply_patch_check(
      "EMMC:/boot.img:src_size:src_hash:tgt_size:tgt_hash",
      "src_hash");

Assume "/boot.img" already has the desired hash of "tgt_hash", the
previous code would give wrong verification result. The issue can be
addressed by additionally listing "tgt_hash" as one of the desired SHAs
(or by applying this CL).

Bug: 110106408
Test: Run recovery_unit_test and recovery_component_test on marlin.
Change-Id: I8daafdbecd083f687e24d563ab089caa25667633

applypatch/applypatch.cpp

@@ -376,24 +376,26 @@ static int FindMatchingPatch(const uint8_t* sha1, const std::vector<std::string>
   return -1;
 }
 
-int applypatch_check(const char* filename, const std::vector<std::string>& patch_sha1s) {
+int applypatch_check(const std::string& filename, const std::vector<std::string>& sha1s) {
-  // It's okay to specify no SHA-1s; the check will pass if the LoadFileContents is successful.
+  if (!android::base::StartsWith(filename, "EMMC:")) {
-  // (Useful for reading partitions, where the filename encodes the SHA-1s; no need to check them
+    return 1;
-  // twice.)
+  }
+
+  // The check will pass if LoadPartitionContents is successful, because the filename already
+  // encodes the desired SHA-1s.
   FileContents file;
-  if (LoadFileContents(filename, &file) != 0 ||
+  if (LoadPartitionContents(filename, &file) != 0) {
-      (!patch_sha1s.empty() && FindMatchingPatch(file.sha1, patch_sha1s) < 0)) {
     LOG(INFO) << "\"" << filename << "\" doesn't have any of expected SHA-1 sums; checking cache";
 
-    // If the source file is missing or corrupted, it might be because we were killed in the middle
+    // If the partition is corrupted, it might be because we were killed in the middle of patching
-    // of patching it. A copy should have been made in cache_temp_source. If that file exists and
+    // it. A copy should have been made in cache_temp_source. If that file exists and matches the
-    // matches the SHA-1 we're looking for, the check still passes.
+    // SHA-1 we're looking for, the check still passes.
     if (LoadFileContents(Paths::Get().cache_temp_source(), &file) != 0) {
       LOG(ERROR) << "Failed to load cache file";
       return 1;
     }
 
-    if (FindMatchingPatch(file.sha1, patch_sha1s) < 0) {
+    if (FindMatchingPatch(file.sha1, sha1s) < 0) {
       LOG(ERROR) << "The cache bits don't match any SHA-1 for \"" << filename << "\"";
       return 1;
     }

applypatch/include/applypatch/applypatch.h

@@ -78,9 +78,10 @@ int applypatch(const char* source_filename, const char* target_filename,
                const std::vector<std::string>& patch_sha1s,
                const std::vector<std::unique_ptr<Value>>& patch_data, const Value* bonus_data);
 
-// Returns 0 if the contents of the file or the cached file match any of the given SHA-1's. Returns
+// Returns 0 if the contents of the eMMC target or the cached file match any of the given SHA-1's.
-// nonzero otherwise.
+// Returns nonzero otherwise. 'filename' must refer to an eMMC partition target. It would only use
-int applypatch_check(const char* filename, const std::vector<std::string>& patch_sha1s);
+// 'sha1s' to find a match on /cache if the hashes embedded in the filename fail to match.
+int applypatch_check(const std::string& filename, const std::vector<std::string>& sha1s);
 
 // Flashes a given image to the target partition. It verifies the target cheksum first, and will
 // return if target already has the desired hash. Otherwise it checks the checksum of the given

tests/component/updater_test.cpp

@@ -250,8 +250,10 @@ TEST_F(UpdaterTest, apply_patch_check) {
   expect("t", cmd.c_str(), kNoCause);
 
   // Multiple arguments.
+  // As long as it successfully loads the partition specified in filename, it won't check against
+  // any given SHAs.
   cmd = "apply_patch_check(\"" + filename + "\", \"wrong_sha1\", \"wrong_sha2\")";
-  expect("", cmd.c_str(), kNoCause);
+  expect("t", cmd.c_str(), kNoCause);
 
   cmd = "apply_patch_check(\"" + filename + "\", \"wrong_sha1\", \"" + src_hash +
         "\", \"wrong_sha2\")";

tests/unit/applypatch_test.cpp

@@ -33,7 +33,6 @@
 #include <android-base/test_utils.h>
 #include <android-base/unique_fd.h>
 #include <gtest/gtest.h>
-#include <openssl/sha.h>
 
 #include "applypatch/applypatch.h"
 #include "common/test_constants.h"
@@ -42,139 +41,102 @@
 
 using namespace std::string_literals;
 
-static void sha1sum(const std::string& fname, std::string* sha1, size_t* fsize = nullptr) {
-  ASSERT_TRUE(sha1 != nullptr);
-
-  std::string data;
-  ASSERT_TRUE(android::base::ReadFileToString(fname, &data));
-
-  if (fsize != nullptr) {
-    *fsize = data.size();
-  }
-
-  uint8_t digest[SHA_DIGEST_LENGTH];
-  SHA1(reinterpret_cast<const uint8_t*>(data.c_str()), data.size(), digest);
-  *sha1 = print_sha1(digest);
-}
-
-static void mangle_file(const std::string& fname) {
-  std::string content(1024, '\0');
-  for (size_t i = 0; i < 1024; i++) {
-    content[i] = rand() % 256;
-  }
-  ASSERT_TRUE(android::base::WriteStringToFile(content, fname));
-}
-
 class ApplyPatchTest : public ::testing::Test {
- public:
+ protected:
   void SetUp() override {
-    // set up files
     old_file = from_testdata_base("old.file");
-    new_file = from_testdata_base("new.file");
+    FileContents old_fc;
-    nonexistent_file = from_testdata_base("nonexistent.file");
+    ASSERT_EQ(0, LoadFileContents(old_file, &old_fc));
+    old_sha1 = print_sha1(old_fc.sha1);
+    old_size = old_fc.data.size();
+
+    new_file = from_testdata_base("new.file");
+    FileContents new_fc;
+    ASSERT_EQ(0, LoadFileContents(new_file, &new_fc));
+    new_sha1 = print_sha1(new_fc.sha1);
+    new_size = new_fc.data.size();
 
-    // set up SHA constants
-    sha1sum(old_file, &old_sha1, &old_size);
-    sha1sum(new_file, &new_sha1, &new_size);
     srand(time(nullptr));
     bad_sha1_a = android::base::StringPrintf("%040x", rand());
     bad_sha1_b = android::base::StringPrintf("%040x", rand());
+
+    // Reset the cache backup file.
+    Paths::Get().set_cache_temp_source("/cache/saved.file");
   }
 
   std::string old_file;
-  std::string new_file;
-  std::string nonexistent_file;
-
   std::string old_sha1;
+  size_t old_size;
+
+  std::string new_file;
   std::string new_sha1;
+  size_t new_size;
+
   std::string bad_sha1_a;
   std::string bad_sha1_b;
-
-  size_t old_size;
-  size_t new_size;
 };
 
-TEST_F(ApplyPatchTest, CheckModeSkip) {
+TEST_F(ApplyPatchTest, CheckMode) {
-  std::vector<std::string> sha1s;
+  std::string partition = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + old_sha1;
-  ASSERT_EQ(0, applypatch_check(&old_file[0], sha1s));
+  ASSERT_EQ(0, applypatch_check(partition, {}));
+  ASSERT_EQ(0, applypatch_check(partition, { old_sha1 }));
+  ASSERT_EQ(0, applypatch_check(partition, { bad_sha1_a, bad_sha1_b }));
+  ASSERT_EQ(0, applypatch_check(partition, { bad_sha1_a, old_sha1, bad_sha1_b }));
 }
 
-TEST_F(ApplyPatchTest, CheckModeSingle) {
+TEST_F(ApplyPatchTest, CheckMode_NonEmmcTarget) {
-  std::vector<std::string> sha1s = { old_sha1 };
+  ASSERT_NE(0, applypatch_check(old_file, {}));
-  ASSERT_EQ(0, applypatch_check(&old_file[0], sha1s));
+  ASSERT_NE(0, applypatch_check(old_file, { old_sha1 }));
+  ASSERT_NE(0, applypatch_check(old_file, { bad_sha1_a, bad_sha1_b }));
+  ASSERT_NE(0, applypatch_check(old_file, { bad_sha1_a, old_sha1, bad_sha1_b }));
 }
 
-TEST_F(ApplyPatchTest, CheckModeMultiple) {
+TEST_F(ApplyPatchTest, CheckMode_EmmcTarget) {
-  std::vector<std::string> sha1s = { bad_sha1_a, old_sha1, bad_sha1_b };
-  ASSERT_EQ(0, applypatch_check(&old_file[0], sha1s));
-}
-
-TEST_F(ApplyPatchTest, CheckModeFailure) {
-  std::vector<std::string> sha1s = { bad_sha1_a, bad_sha1_b };
-  ASSERT_NE(0, applypatch_check(&old_file[0], sha1s));
-}
-
-TEST_F(ApplyPatchTest, CheckModeEmmcTarget) {
   // EMMC:old_file:size:sha1 should pass the check.
   std::string src_file = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + old_sha1;
-  std::vector<std::string> sha1s;
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
 
   // EMMC:old_file:(size-1):sha1:(size+1):sha1 should fail the check.
   src_file = "EMMC:" + old_file + ":" + std::to_string(old_size - 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size + 1) + ":" + old_sha1;
-  ASSERT_EQ(1, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_NE(0, applypatch_check(src_file, {}));
 
   // EMMC:old_file:(size-1):sha1:size:sha1:(size+1):sha1 should pass the check.
   src_file = "EMMC:" + old_file + ":" + std::to_string(old_size - 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size) + ":" + old_sha1 + ":" + std::to_string(old_size + 1) + ":" +
              old_sha1;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 
   // EMMC:old_file:(size+1):sha1:(size-1):sha1:size:sha1 should pass the check.
   src_file = "EMMC:" + old_file + ":" + std::to_string(old_size + 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size - 1) + ":" + old_sha1 + ":" + std::to_string(old_size) + ":" +
              old_sha1;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 
   // EMMC:new_file:(size+1):old_sha1:(size-1):old_sha1:size:old_sha1:size:new_sha1
   // should pass the check.
   src_file = "EMMC:" + new_file + ":" + std::to_string(old_size + 1) + ":" + old_sha1 + ":" +
              std::to_string(old_size - 1) + ":" + old_sha1 + ":" + std::to_string(old_size) + ":" +
              old_sha1 + ":" + std::to_string(new_size) + ":" + new_sha1;
-  ASSERT_EQ(0, applypatch_check(src_file.c_str(), sha1s));
+  ASSERT_EQ(0, applypatch_check(src_file, {}));
 }
 
-class ApplyPatchCacheTest : public ApplyPatchTest {
+TEST_F(ApplyPatchTest, CheckMode_UseBackup) {
- protected:
+  std::string corrupted = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + bad_sha1_a;
-  void SetUp() override {
+  ASSERT_NE(0, applypatch_check(corrupted, { old_sha1 }));
-    ApplyPatchTest::SetUp();
+
   Paths::Get().set_cache_temp_source(old_file);
-  }
+  ASSERT_EQ(0, applypatch_check(corrupted, { old_sha1 }));
-};
+  ASSERT_EQ(0, applypatch_check(corrupted, { bad_sha1_a, old_sha1, bad_sha1_b }));
-
-TEST_F(ApplyPatchCacheTest, CheckCacheCorruptedSourceSingle) {
-  TemporaryFile temp_file;
-  mangle_file(temp_file.path);
-  std::vector<std::string> sha1s_single = { old_sha1 };
-  ASSERT_EQ(0, applypatch_check(temp_file.path, sha1s_single));
-  ASSERT_EQ(0, applypatch_check(nonexistent_file.c_str(), sha1s_single));
 }
 
-TEST_F(ApplyPatchCacheTest, CheckCacheCorruptedSourceMultiple) {
+TEST_F(ApplyPatchTest, CheckMode_UseBackup_BothCorrupted) {
-  TemporaryFile temp_file;
+  std::string corrupted = "EMMC:" + old_file + ":" + std::to_string(old_size) + ":" + bad_sha1_a;
-  mangle_file(temp_file.path);
+  ASSERT_NE(0, applypatch_check(corrupted, {}));
-  std::vector<std::string> sha1s_multiple = { bad_sha1_a, old_sha1, bad_sha1_b };
+  ASSERT_NE(0, applypatch_check(corrupted, { old_sha1 }));
-  ASSERT_EQ(0, applypatch_check(temp_file.path, sha1s_multiple));
-  ASSERT_EQ(0, applypatch_check(nonexistent_file.c_str(), sha1s_multiple));
-}
 
-TEST_F(ApplyPatchCacheTest, CheckCacheCorruptedSourceFailure) {
+  Paths::Get().set_cache_temp_source(old_file);
-  TemporaryFile temp_file;
+  ASSERT_NE(0, applypatch_check(corrupted, { bad_sha1_a, bad_sha1_b }));
-  mangle_file(temp_file.path);
-  std::vector<std::string> sha1s_failure = { bad_sha1_a, bad_sha1_b };
-  ASSERT_NE(0, applypatch_check(temp_file.path, sha1s_failure));
-  ASSERT_NE(0, applypatch_check(nonexistent_file.c_str(), sha1s_failure));
 }
 
 class FreeCacheTest : public ::testing::Test {