Remove dependency between apex_file and apex_preinstalled_data
The only reason that dependency was there is to get a trusted public key
when validating vbmeta of an apex. This responsibility is now delegated
to the caller of ValidateApexVerity.
Test: atest ApexTestCases
Bug: 165948777
Change-Id: Ibfff81c4dfd078546d3dcf3167ad624e8028a52b
Merged-In: Ibfff81c4dfd078546d3dcf3167ad624e8028a52b
(cherry picked from commit 814ca221e8862ad9db72838a23fa673c4201d8a0)
diff --git a/apexd/Android.bp b/apexd/Android.bp
index 4f0d765..e9e5470 100644
--- a/apexd/Android.bp
+++ b/apexd/Android.bp
@@ -207,6 +207,7 @@
static: {
whole_static_libs: ["libc++fs"],
},
+ cpp_std: "experimental",
shared: {
static_libs: ["libc++fs"],
},
diff --git a/apexd/apex_file.cpp b/apexd/apex_file.cpp
index 36fd7c9..282595a 100644
--- a/apexd/apex_file.cpp
+++ b/apexd/apex_file.cpp
@@ -23,6 +23,7 @@
#include <filesystem>
#include <fstream>
+#include <span>
#include <android-base/file.h>
#include <android-base/logging.h>
@@ -31,18 +32,16 @@
#include <android-base/unique_fd.h>
#include <google/protobuf/util/message_differencer.h>
#include <libavb/libavb.h>
+#include <ziparchive/zip_archive.h>
#include "apex_constants.h"
-#include "apex_preinstalled_data.h"
#include "apexd_utils.h"
-#include "string_log.h"
using android::base::borrowed_fd;
using android::base::EndsWith;
using android::base::Error;
using android::base::ReadFullyAtOffset;
using android::base::Result;
-using android::base::StartsWith;
using android::base::unique_fd;
using google::protobuf::util::MessageDifferencer;
@@ -148,7 +147,7 @@
}
return ApexFile(path, image_offset, image_size, std::move(*manifest), pubkey,
- isPathForBuiltinApexes(path), *fs_type);
+ *fs_type);
}
// AVB-related code.
@@ -214,8 +213,10 @@
memcmp(&public_key_content[0], key, length) == 0;
}
-Result<void> verifyVbMetaSignature(const ApexFile& apex, const uint8_t* data,
- size_t length) {
+// Verifies correctness of vbmeta and returns public key it was signed with.
+Result<std::span<const uint8_t>> verifyVbMetaSignature(const ApexFile& apex,
+ const uint8_t* data,
+ size_t length) {
const uint8_t* pk;
size_t pk_len;
AvbVBMetaVerifyResult res;
@@ -236,31 +237,16 @@
return Error() << "Error verifying " << apex.GetPath() << ": "
<< "unsupported version";
default:
- return Errorf("Unknown vmbeta_image_verify return value");
+ return Error() << "Unknown vmbeta_image_verify return value : " << res;
}
- auto& instance = ApexPreinstalledData::GetInstance();
- // TODO(b/165948777): extract into verifier.
- Result<const std::string> public_key =
- instance.GetPublicKey(apex.GetManifest().name());
- if (public_key.ok()) {
- // TODO(b/115718846)
- // We need to decide whether we need rollback protection, and whether
- // we can use the rollback protection provided by libavb.
- if (!CompareKeys(pk, pk_len, *public_key)) {
- return Error() << "Error verifying " << apex.GetPath() << ": "
- << "public key doesn't match the pre-installed one";
- }
- } else {
- return public_key.error();
- }
- LOG(VERBOSE) << apex.GetPath() << ": public key matches.";
- return {};
+ return std::span<const uint8_t>(pk, pk_len);
}
Result<std::unique_ptr<uint8_t[]>> verifyVbMeta(const ApexFile& apex,
const unique_fd& fd,
- const AvbFooter& footer) {
+ const AvbFooter& footer,
+ const std::string& public_key) {
if (footer.vbmeta_size > kVbMetaMaxSize) {
return Errorf("VbMeta size in footer exceeds kVbMetaMaxSize.");
}
@@ -272,12 +258,17 @@
return ErrnoError() << "Couldn't read AVB meta-data";
}
- Result<void> st =
+ Result<std::span<const uint8_t>> st =
verifyVbMetaSignature(apex, vbmeta_buf.get(), footer.vbmeta_size);
if (!st.ok()) {
return st.error();
}
+ if (!CompareKeys(st->data(), st->size(), public_key)) {
+ return Error() << "Error verifying " << apex.GetPath() << " : "
+ << "public key doesn't match the pre-installed one";
+ }
+
return vbmeta_buf;
}
@@ -329,7 +320,8 @@
} // namespace
-Result<ApexVerityData> ApexFile::VerifyApexVerity() const {
+Result<ApexVerityData> ApexFile::VerifyApexVerity(
+ const std::string& public_key) const {
ApexVerityData verityData;
unique_fd fd(open(GetPath().c_str(), O_RDONLY | O_CLOEXEC));
@@ -343,7 +335,7 @@
}
Result<std::unique_ptr<uint8_t[]>> vbmeta_data =
- verifyVbMeta(*this, fd, **footer);
+ verifyVbMeta(*this, fd, **footer, public_key);
if (!vbmeta_data.ok()) {
return vbmeta_data.error();
}
@@ -420,14 +412,5 @@
return ReadDir(path, filter_fn);
}
-bool isPathForBuiltinApexes(const std::string& path) {
- for (const auto& dir : kApexPackageBuiltinDirs) {
- if (StartsWith(path, dir)) {
- return true;
- }
- }
- return false;
-}
-
} // namespace apex
} // namespace android
diff --git a/apexd/apex_file.h b/apexd/apex_file.h
index 88914a9..8647807 100644
--- a/apexd/apex_file.h
+++ b/apexd/apex_file.h
@@ -23,9 +23,7 @@
#include <android-base/result.h>
#include <libavb/libavb.h>
-#include <ziparchive/zip_archive.h>
-#include "apex_constants.h"
#include "apex_manifest.h"
namespace android {
@@ -52,23 +50,22 @@
size_t GetImageSize() const { return image_size_; }
const ApexManifest& GetManifest() const { return manifest_; }
const std::string& GetBundledPublicKey() const { return apex_pubkey_; }
- bool IsBuiltin() const { return is_builtin_; }
const std::string& GetFsType() const { return fs_type_; }
- android::base::Result<ApexVerityData> VerifyApexVerity() const;
+ android::base::Result<ApexVerityData> VerifyApexVerity(
+ const std::string& public_key) const;
+ // TODO(b/165948777): this doesn't seem to belong to ApexFile?
android::base::Result<void> VerifyManifestMatches(
const std::string& mount_path) const;
private:
ApexFile(const std::string& apex_path, int32_t image_offset,
size_t image_size, ApexManifest manifest,
- const std::string& apex_pubkey, bool is_builtin,
- const std::string& fs_type)
+ const std::string& apex_pubkey, const std::string& fs_type)
: apex_path_(apex_path),
image_offset_(image_offset),
image_size_(image_size),
manifest_(std::move(manifest)),
apex_pubkey_(apex_pubkey),
- is_builtin_(is_builtin),
fs_type_(fs_type) {}
std::string apex_path_;
@@ -76,17 +73,15 @@
size_t image_size_;
ApexManifest manifest_;
std::string apex_pubkey_;
- bool is_builtin_;
std::string fs_type_;
};
+// TODO(b/165948777): this doesn't seem to belong to apex_file.h
android::base::Result<std::vector<std::string>> FindApexes(
const std::vector<std::string>& paths);
android::base::Result<std::vector<std::string>> FindApexFilesByName(
const std::string& path);
-bool isPathForBuiltinApexes(const std::string& path);
-
} // namespace apex
} // namespace android
diff --git a/apexd/apex_file_test.cpp b/apexd/apex_file_test.cpp
index 8555e47..2cf7d5d 100644
--- a/apexd/apex_file_test.cpp
+++ b/apexd/apex_file_test.cpp
@@ -26,7 +26,6 @@
#include <ziparchive/zip_archive.h>
#include "apex_file.h"
-#include "apex_preinstalled_data.h"
using android::base::Result;
@@ -93,13 +92,11 @@
}
TEST_P(ApexFileTest, VerifyApexVerity) {
- ApexPreinstalledData& instance = ApexPreinstalledData::GetInstance();
- ASSERT_RESULT_OK(instance.Initialize({"/system_ext/apex"}));
const std::string filePath = testDataDir + GetParam().prefix + ".apex";
Result<ApexFile> apexFile = ApexFile::Open(filePath);
ASSERT_RESULT_OK(apexFile);
- auto verity_or = apexFile->VerifyApexVerity();
+ auto verity_or = apexFile->VerifyApexVerity(apexFile->GetBundledPublicKey());
ASSERT_RESULT_OK(verity_or);
const ApexVerityData& data = *verity_or;
@@ -118,13 +115,12 @@
EXPECT_EQ(std::string(rootDigest), data.root_digest);
}
-TEST_P(ApexFileTest, VerifyApexVerityNoKeyInst) {
- const std::string filePath =
- testDataDir + GetParam().prefix + "_no_inst_key.apex";
+TEST_P(ApexFileTest, VerifyApexVerityWrongKey) {
+ const std::string filePath = testDataDir + GetParam().prefix + ".apex";
Result<ApexFile> apexFile = ApexFile::Open(filePath);
ASSERT_RESULT_OK(apexFile);
- auto verity_or = apexFile->VerifyApexVerity();
+ auto verity_or = apexFile->VerifyApexVerity("wrong-key");
ASSERT_FALSE(verity_or.ok());
}
@@ -146,7 +142,7 @@
const std::string apex_path = testDataDir + "corrupted_b146895998.apex";
Result<ApexFile> apex = ApexFile::Open(apex_path);
ASSERT_RESULT_OK(apex);
- ASSERT_FALSE(apex->VerifyApexVerity());
+ ASSERT_FALSE(apex->VerifyApexVerity("ignored"));
}
TEST_P(ApexFileTest, RetrieveFsType) {
diff --git a/apexd/apex_preinstalled_data.cpp b/apexd/apex_preinstalled_data.cpp
index dea5336..827566f 100644
--- a/apexd/apex_preinstalled_data.cpp
+++ b/apexd/apex_preinstalled_data.cpp
@@ -110,5 +110,13 @@
return data_.find(name) != data_.end();
}
+bool ApexPreinstalledData::IsPreInstalledApex(const ApexFile& apex) const {
+ auto it = data_.find(apex.GetManifest().name());
+ if (it == data_.end()) {
+ return false;
+ }
+ return it->second.path == apex.GetPath();
+}
+
} // namespace apex
} // namespace android
diff --git a/apexd/apex_preinstalled_data.h b/apexd/apex_preinstalled_data.h
index 59aeebc..44ac32b 100644
--- a/apexd/apex_preinstalled_data.h
+++ b/apexd/apex_preinstalled_data.h
@@ -19,6 +19,7 @@
#include <string>
#include <unordered_map>
#include <vector>
+#include "apex_file.h"
#include <android-base/result.h>
@@ -60,6 +61,9 @@
// |name|.
bool HasPreInstalledVersion(const std::string& name) const;
+ // Checks if given |apex| is pre-installed.
+ bool IsPreInstalledApex(const ApexFile& apex) const;
+
private:
// Non-copyable && non-moveable.
ApexPreinstalledData(const ApexPreinstalledData&) = delete;
diff --git a/apexd/apex_preinstalled_data_test.cpp b/apexd/apex_preinstalled_data_test.cpp
index 0e9de06..8efe68a 100644
--- a/apexd/apex_preinstalled_data_test.cpp
+++ b/apexd/apex_preinstalled_data_test.cpp
@@ -106,5 +106,29 @@
HasSubstr("does not match with previously scanned key"));
}
+TEST(ApexPreinstalledData, IsPreInstalledApex) {
+ // Prepare test data.
+ TemporaryDir td;
+ fs::copy(GetTestFile("apex.apexd_test.apex"), td.path);
+
+ ApexPreinstalledData instance;
+ ASSERT_TRUE(IsOk(instance.Initialize({td.path})));
+
+ auto apex1 = ApexFile::Open(StringPrintf("%s/apex.apexd_test.apex", td.path));
+ ASSERT_TRUE(IsOk(apex1));
+ ASSERT_TRUE(instance.IsPreInstalledApex(*apex1));
+
+ // It's same apex, but path is different. Shouldn't be treated as
+ // pre-installed.
+ auto apex2 = ApexFile::Open(GetTestFile("apex.apexd_test.apex"));
+ ASSERT_TRUE(IsOk(apex2));
+ ASSERT_FALSE(instance.IsPreInstalledApex(*apex2));
+
+ auto apex3 =
+ ApexFile::Open(GetTestFile("apex.apexd_test_different_app.apex"));
+ ASSERT_TRUE(IsOk(apex3));
+ ASSERT_FALSE(instance.IsPreInstalledApex(*apex3));
+}
+
} // namespace apex
} // namespace android
diff --git a/apexd/apexd.cpp b/apexd/apexd.cpp
index 0a359e1..3bfaca8 100644
--- a/apexd/apexd.cpp
+++ b/apexd/apexd.cpp
@@ -406,7 +406,14 @@
}
LOG(VERBOSE) << "Loopback device created: " << loopbackDevice.name;
- auto verityData = apex.VerifyApexVerity();
+ auto& instance = ApexPreinstalledData::GetInstance();
+
+ auto public_key = instance.GetPublicKey(apex.GetManifest().name());
+ if (!public_key.ok()) {
+ return public_key.error();
+ }
+
+ auto verityData = apex.VerifyApexVerity(*public_key);
if (!verityData.ok()) {
return Error() << "Failed to verify Apex Verity data for " << full_path
<< ": " << verityData.error();
@@ -421,7 +428,7 @@
// dm-verity because they are already in the dm-verity protected partition;
// system. However, note that we don't skip verification to ensure that APEXes
// are correctly signed.
- const bool mountOnVerity = !isPathForBuiltinApexes(full_path);
+ const bool mountOnVerity = !instance.IsPreInstalledApex(apex);
DmVerityDevice verityDev;
loop::LoopbackDeviceUniqueFd loop_for_hash;
if (mountOnVerity) {
@@ -675,7 +682,13 @@
// This function should only verification checks that are necessary to run on
// each boot. Try to avoid putting expensive checks inside this function.
Result<void> VerifyPackageBoot(const ApexFile& apex_file) {
- Result<ApexVerityData> verity_or = apex_file.VerifyApexVerity();
+ // TODO(ioffe): why do we need this here?
+ auto& instance = ApexPreinstalledData::GetInstance();
+ auto public_key = instance.GetPublicKey(apex_file.GetManifest().name());
+ if (!public_key.ok()) {
+ return public_key.error();
+ }
+ Result<ApexVerityData> verity_or = apex_file.VerifyApexVerity(*public_key);
if (!verity_or.ok()) {
return verity_or.error();
}
@@ -701,7 +714,6 @@
if (!verify_package_boot_status.ok()) {
return verify_package_boot_status;
}
- Result<ApexVerityData> verity_or = apex_file.VerifyApexVerity();
constexpr const auto kSuccessFn = [](const std::string& /*mount_point*/) {
return Result<void>{};
@@ -1121,9 +1133,10 @@
std::vector<com::android::apex::ApexInfo> apexInfos;
auto convertToAutogen = [&apexInfos](const ApexFile& apex, bool isActive) {
+ auto& instance = ApexPreinstalledData::GetInstance();
+
auto preinstalledPath =
- ApexPreinstalledData::GetInstance().GetPreinstalledPath(
- apex.GetManifest().name());
+ instance.GetPreinstalledPath(apex.GetManifest().name());
std::optional<std::string> preinstalledModulePath;
if (preinstalledPath.ok()) {
preinstalledModulePath = *preinstalledPath;
@@ -1131,7 +1144,7 @@
com::android::apex::ApexInfo apexInfo(
apex.GetManifest().name(), apex.GetPath(), preinstalledModulePath,
apex.GetManifest().version(), apex.GetManifest().versionname(),
- apex.IsBuiltin(), isActive);
+ instance.IsPreInstalledApex(apex), isActive);
apexInfos.emplace_back(apexInfo);
};
@@ -1832,11 +1845,12 @@
LOG(DEBUG) << "unstagePackages() for " << Join(paths, ',');
for (const std::string& path : paths) {
- if (isPathForBuiltinApexes(path)) {
- return Error() << "Can't uninstall pre-installed apex " << path;
+ auto apex = ApexFile::Open(path);
+ if (!apex.ok()) {
+ return apex.error();
}
- if (access(path.c_str(), F_OK) != 0) {
- return ErrnoError() << "Can't access " << path;
+ if (ApexPreinstalledData::GetInstance().IsPreInstalledApex(*apex)) {
+ return Error() << "Can't uninstall pre-installed apex " << path;
}
}
diff --git a/apexd/apexd_verity_test.cpp b/apexd/apexd_verity_test.cpp
index 33fd782..f431bd3 100644
--- a/apexd/apexd_verity_test.cpp
+++ b/apexd/apexd_verity_test.cpp
@@ -25,7 +25,6 @@
#include <gtest/gtest.h>
#include "apex_file.h"
-#include "apex_preinstalled_data.h"
#include "apexd_test_utils.h"
#include "apexd_verity.h"
@@ -45,13 +44,11 @@
}
TEST(ApexdVerityTest, ReusesHashtree) {
- ApexPreinstalledData& instance = ApexPreinstalledData::GetInstance();
- ASSERT_TRUE(IsOk(instance.Initialize({"/system_ext/apex"})));
TemporaryDir td;
auto apex = ApexFile::Open(GetTestFile("apex.apexd_test_no_hashtree.apex"));
ASSERT_TRUE(IsOk(apex));
- auto verity_data = apex->VerifyApexVerity();
+ auto verity_data = apex->VerifyApexVerity(apex->GetBundledPublicKey());
ASSERT_TRUE(IsOk(verity_data));
auto hashtree_file = StringPrintf("%s/hashtree", td.path);
@@ -79,13 +76,11 @@
}
TEST(ApexdVerityTest, RegenerateHashree) {
- ApexPreinstalledData& instance = ApexPreinstalledData::GetInstance();
- ASSERT_TRUE(IsOk(instance.Initialize({"/system_ext/apex"})));
TemporaryDir td;
auto apex = ApexFile::Open(GetTestFile("apex.apexd_test_no_hashtree.apex"));
ASSERT_TRUE(IsOk(apex));
- auto verity_data = apex->VerifyApexVerity();
+ auto verity_data = apex->VerifyApexVerity(apex->GetBundledPublicKey());
ASSERT_TRUE(IsOk(verity_data));
auto hashtree_file = StringPrintf("%s/hashtree", td.path);
@@ -100,7 +95,7 @@
auto apex2 =
ApexFile::Open(GetTestFile("apex.apexd_test_no_hashtree_2.apex"));
ASSERT_TRUE(IsOk(apex2));
- auto verity_data2 = apex2->VerifyApexVerity();
+ auto verity_data2 = apex2->VerifyApexVerity(apex2->GetBundledPublicKey());
ASSERT_TRUE(IsOk(verity_data2));
// Now call PrepareHashTree again. Since digest doesn't match, hashtree
diff --git a/apexd/apexservice.cpp b/apexd/apexservice.cpp
index 75bead0..c900bb7 100644
--- a/apexd/apexservice.cpp
+++ b/apexd/apexservice.cpp
@@ -267,16 +267,16 @@
}
static ApexInfo getApexInfo(const ApexFile& package) {
+ auto& instance = ApexPreinstalledData::GetInstance();
ApexInfo out;
out.moduleName = package.GetManifest().name();
out.modulePath = package.GetPath();
out.versionCode = package.GetManifest().version();
out.versionName = package.GetManifest().versionname();
- out.isFactory = package.IsBuiltin();
+ out.isFactory = instance.IsPreInstalledApex(package);
out.isActive = false;
Result<std::string> preinstalledPath =
- ApexPreinstalledData::GetInstance().GetPreinstalledPath(
- package.GetManifest().name());
+ instance.GetPreinstalledPath(package.GetManifest().name());
if (preinstalledPath.ok()) {
out.preinstalledModulePath = *preinstalledPath;
}
diff --git a/apexd/apexservice_test.cpp b/apexd/apexservice_test.cpp
index a66df5f..17c4438 100644
--- a/apexd/apexservice_test.cpp
+++ b/apexd/apexservice_test.cpp
@@ -1384,7 +1384,13 @@
ASSERT_TRUE(factoryPackages->size() > 0);
for (const ApexInfo& package : *factoryPackages) {
- ASSERT_TRUE(isPathForBuiltinApexes(package.modulePath));
+ bool is_builtin = false;
+ for (const auto& dir : kApexPackageBuiltinDirs) {
+ if (StartsWith(package.modulePath, dir)) {
+ is_builtin = true;
+ }
+ }
+ ASSERT_TRUE(is_builtin);
}
}
