From e68f6cd6c0a925fea1bbe7537f6029ef9b0e142c Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 1 Oct 2024 11:01:08 -0700 Subject: [PATCH 1/4] libdm: Redact keys from dm-crypt targets when calling GetTable. Ignore-AOSP-First: security fix Bug: 368069390 Test: libdm_test Change-Id: I40b9a0129e58b1a0f116ca29f0ee66f91a27a73d Merged-In: I40b9a0129e58b1a0f116ca29f0ee66f91a27a73d --- fs_mgr/libdm/dm.cpp | 14 ++++++++++++++ fs_mgr/libdm/dm_test.cpp | 29 +++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/fs_mgr/libdm/dm.cpp b/fs_mgr/libdm/dm.cpp index b1d5b397a..762e83c30 100644 --- a/fs_mgr/libdm/dm.cpp +++ b/fs_mgr/libdm/dm.cpp @@ -512,6 +512,17 @@ bool DeviceMapper::GetTableInfo(const std::string& name, std::vector return GetTable(name, DM_STATUS_TABLE_FLAG, table); } +void RedactTableInfo(const struct dm_target_spec& spec, std::string* data) { + if (DeviceMapper::GetTargetType(spec) == "crypt") { + auto parts = android::base::Split(*data, " "); + if (parts.size() < 2) { + return; + } + parts[1] = "redacted"; + *data = android::base::Join(parts, " "); + } +} + // private methods of DeviceMapper bool DeviceMapper::GetTable(const std::string& name, uint32_t flags, std::vector* table) { @@ -550,6 +561,9 @@ bool DeviceMapper::GetTable(const std::string& name, uint32_t flags, // Note: we use c_str() to eliminate any extra trailing 0s. data = std::string(&buffer[data_offset], next_cursor - data_offset).c_str(); } + if (flags & DM_STATUS_TABLE_FLAG) { + RedactTableInfo(*spec, &data); + } table->emplace_back(*spec, data); cursor = next_cursor; } diff --git a/fs_mgr/libdm/dm_test.cpp b/fs_mgr/libdm/dm_test.cpp index 541f254cb..f4c9784da 100644 --- a/fs_mgr/libdm/dm_test.cpp +++ b/fs_mgr/libdm/dm_test.cpp @@ -690,3 +690,32 @@ TEST(libdm, CreateEmptyDevice) { // Empty device should be in suspended state. ASSERT_EQ(DmDeviceState::SUSPENDED, dm.GetState("empty-device")); } + +TEST(libdm, RedactDmCrypt) { + static constexpr uint64_t kImageSize = 65536; + static constexpr const char* kTestName = "RedactDmCrypt"; + unique_fd temp_file(CreateTempFile("file_1", kImageSize)); + ASSERT_GE(temp_file, 0); + + LoopDevice loop(temp_file, 10s); + ASSERT_TRUE(loop.valid()); + + static constexpr const char* kAlgorithm = "aes-cbc-essiv:sha256"; + static constexpr const char* kKey = "0e64ef514e6a1315b1f6390cb57c9e6a"; + + auto target = std::make_unique(0, kImageSize / 512, kAlgorithm, kKey, 0, + loop.device(), 0); + target->AllowDiscards(); + + DmTable table; + table.AddTarget(std::move(target)); + + auto& dm = DeviceMapper::Instance(); + std::string crypt_path; + ASSERT_TRUE(dm.CreateDevice(kTestName, table, &crypt_path, 10s)); + + std::vector targets; + ASSERT_TRUE(dm.GetTableInfo(kTestName, &targets)); + ASSERT_EQ(targets.size(), 1); + EXPECT_EQ(targets[0].data.find(kKey), std::string::npos); +} From c9e000c7acf8b9a011491424c3e9b6d667c58221 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 1 Oct 2024 11:01:08 -0700 Subject: [PATCH 2/4] libdm: Redact keys from dm-crypt targets when calling GetTable. Ignore-AOSP-First: security fix Bug: 368069390 Test: libdm_test Change-Id: I40b9a0129e58b1a0f116ca29f0ee66f91a27a73d Merged-In: I40b9a0129e58b1a0f116ca29f0ee66f91a27a73d --- fs_mgr/libdm/dm.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/fs_mgr/libdm/dm.cpp b/fs_mgr/libdm/dm.cpp index e43c00b44..dbee8a377 100644 --- a/fs_mgr/libdm/dm.cpp +++ b/fs_mgr/libdm/dm.cpp @@ -494,6 +494,17 @@ bool DeviceMapper::GetTableInfo(const std::string& name, std::vector return GetTable(name, DM_STATUS_TABLE_FLAG, table); } +void RedactTableInfo(const struct dm_target_spec& spec, std::string* data) { + if (DeviceMapper::GetTargetType(spec) == "crypt") { + auto parts = android::base::Split(*data, " "); + if (parts.size() < 2) { + return; + } + parts[1] = "redacted"; + *data = android::base::Join(parts, " "); + } +} + // private methods of DeviceMapper bool DeviceMapper::GetTable(const std::string& name, uint32_t flags, std::vector* table) { @@ -532,6 +543,9 @@ bool DeviceMapper::GetTable(const std::string& name, uint32_t flags, // Note: we use c_str() to eliminate any extra trailing 0s. data = std::string(&buffer[data_offset], next_cursor - data_offset).c_str(); } + if (flags & DM_STATUS_TABLE_FLAG) { + RedactTableInfo(*spec, &data); + } table->emplace_back(*spec, data); cursor = next_cursor; } From 5f216ffdc35266acf175db0e73e952f2bca80c39 Mon Sep 17 00:00:00 2001 From: Armelle Laine Date: Sun, 6 Oct 2024 05:40:40 +0000 Subject: [PATCH 3/4] trusty: utils: rpmb_dev: add wv secure storage init.rc Bug: 371777025 Change-Id: Id4f26509568dac1045e0b2ba58a045874555a303 Test: cuttlefish with trusty-vm enablement apex, run WV VTS --- trusty/utils/rpmb_dev/Android.bp | 9 +++ trusty/utils/rpmb_dev/rpmb_dev.wv.system.rc | 62 +++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 trusty/utils/rpmb_dev/rpmb_dev.wv.system.rc diff --git a/trusty/utils/rpmb_dev/Android.bp b/trusty/utils/rpmb_dev/Android.bp index 13f151d2e..ef23cc50f 100644 --- a/trusty/utils/rpmb_dev/Android.bp +++ b/trusty/utils/rpmb_dev/Android.bp @@ -49,3 +49,12 @@ cc_binary { "rpmb_dev.system.rc", ], } + +cc_binary { + name: "rpmb_dev.wv.system", + defaults: ["rpmb_dev.cc_defaults"], + system_ext_specific: true, + init_rc: [ + "rpmb_dev.wv.system.rc", + ], +} diff --git a/trusty/utils/rpmb_dev/rpmb_dev.wv.system.rc b/trusty/utils/rpmb_dev/rpmb_dev.wv.system.rc new file mode 100644 index 000000000..3e7f8b44f --- /dev/null +++ b/trusty/utils/rpmb_dev/rpmb_dev.wv.system.rc @@ -0,0 +1,62 @@ +service storageproxyd_wv_system /system_ext/bin/storageproxyd.system \ + -d ${storageproxyd_wv_system.trusty_ipc_dev:-/dev/trusty-ipc-dev0} \ + -r /dev/socket/rpmb_mock_wv_system \ + -p /data/secure_storage_wv_system \ + -t sock + disabled + class hal + user system + group system + +service rpmb_mock_init_wv_system /system_ext/bin/rpmb_dev.wv.system \ + --dev /mnt/secure_storage_rpmb_wv_system/persist/RPMB_DATA --init --size 2048 + disabled + user system + group system + oneshot + +service rpmb_mock_wv_system /system_ext/bin/rpmb_dev.wv.system \ + --dev /mnt/secure_storage_rpmb_wv_system/persist/RPMB_DATA \ + --sock rpmb_mock_wv_system + disabled + user system + group system + socket rpmb_mock_wv_system stream 660 system system + +# storageproxyd +on boot && \ + property:trusty.widevine_vm.nonsecure_vm_ready=1 && \ + property:storageproxyd_wv_system.trusty_ipc_dev=* + wait /dev/socket/rpmb_mock_wv_system + enable storageproxyd_wv_system + + +# RPMB Mock +on early-boot && \ + property:ro.hardware.security.trusty.widevine_vm.system=1 && \ + property:trusty.widevine_vm.vm_cid=* && \ + property:ro.boot.vendor.apex.com.android.services.widevine=\ +com.android.services.widevine.cf_guest_trusty_nonsecure + # Create a persistent location for the RPMB data + # (work around lack of RPMb block device on CF). + # file contexts secure_storage_rpmb_system_file + # (only used on Cuttlefish as this is non secure) + mkdir /metadata/secure_storage_rpmb_wv_system 0770 system system + mkdir /mnt/secure_storage_rpmb_wv_system 0770 system system + symlink /metadata/secure_storage_rpmb_wv_system \ + /mnt/secure_storage_rpmb_wv_system/persist + # Create a system persist directory in /metadata + # (work around lack of dedicated system persist partition). + # file contexts secure_storage_persist_system_file + mkdir /metadata/secure_storage_persist_wv_system 0770 system system + mkdir /mnt/secure_storage_persist_wv_system 0770 system system + symlink /metadata/secure_storage_persist_wv_system \ + /mnt/secure_storage_persist_wv_system/persist + # file contexts secure_storage_system_file + mkdir /data/secure_storage_wv_system 0770 root system + symlink /mnt/secure_storage_persist_wv_system/persist \ + /data/secure_storage_wv_system/persist + chown root system /data/secure_storage_wv_system/persist + setprop storageproxyd_wv_system.trusty_ipc_dev VSOCK:${trusty.widevine_vm.vm_cid}:1 + exec_start rpmb_mock_init_wv_system + start rpmb_mock_wv_system From 5996d608af75755f0c1964e62fe69482761efd24 Mon Sep 17 00:00:00 2001 From: Chan Wang Date: Thu, 14 Nov 2024 13:25:37 +0000 Subject: [PATCH 4/4] Use the new 'partition' field in 'ApexInfo' to identify vendor apexes A new field 'partition' was added to `ApexInfo` recently which stores pre-installed partition information as string (e.g. 'SYSTEM') in aosp/3335753. Using 'partition' field for Subcontext vendor apex initialization because the existing field `preinstalledModulePath` won't be populated for brand-new apex (a new type we introduced recently). Bug: 377111286 Test: atest CtsInitTestCases Change-Id: I8970b3cb5884bdb949035f5bdc5b2e18618cc9cc --- init/init.cpp | 3 +-- init/subcontext.cpp | 11 ++++++++--- init/subcontext.h | 6 +++++- init/subcontext_benchmark.cpp | 2 +- init/subcontext_test.cpp | 15 ++++++++++++++- 5 files changed, 29 insertions(+), 8 deletions(-) diff --git a/init/init.cpp b/init/init.cpp index 17498da6d..5b0b0ddee 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -315,8 +315,7 @@ Parser CreateApexConfigParser(ActionManager& action_manager, ServiceList& servic if (apex_info_list.has_value()) { std::vector subcontext_apexes; for (const auto& info : apex_info_list->getApexInfo()) { - if (info.hasPreinstalledModulePath() && - subcontext->PathMatchesSubcontext(info.getPreinstalledModulePath())) { + if (subcontext->PartitionMatchesSubcontext(info.getPartition())) { subcontext_apexes.push_back(info.getModuleName()); } } diff --git a/init/subcontext.cpp b/init/subcontext.cpp index 6a095fb7b..3fe448fe3 100644 --- a/init/subcontext.cpp +++ b/init/subcontext.cpp @@ -263,6 +263,10 @@ bool Subcontext::PathMatchesSubcontext(const std::string& path) const { return false; } +bool Subcontext::PartitionMatchesSubcontext(const std::string& partition) const { + return std::find(partitions_.begin(), partitions_.end(), partition) != partitions_.end(); +} + void Subcontext::SetApexList(std::vector&& apex_list) { apex_list_ = std::move(apex_list); } @@ -352,12 +356,13 @@ void InitializeSubcontext() { } if (SelinuxGetVendorAndroidVersion() >= __ANDROID_API_P__) { - subcontext.reset( - new Subcontext(std::vector{"/vendor", "/odm"}, kVendorContext)); + subcontext.reset(new Subcontext(std::vector{"/vendor", "/odm"}, + std::vector{"VENDOR", "ODM"}, kVendorContext)); } } + void InitializeHostSubcontext(std::vector vendor_prefixes) { - subcontext.reset(new Subcontext(vendor_prefixes, kVendorContext, /*host=*/true)); + subcontext.reset(new Subcontext(vendor_prefixes, {}, kVendorContext, /*host=*/true)); } Subcontext* GetSubcontext() { diff --git a/init/subcontext.h b/init/subcontext.h index 93ebacea2..23c4a241c 100644 --- a/init/subcontext.h +++ b/init/subcontext.h @@ -36,8 +36,10 @@ static constexpr const char kTestContext[] = "test-test-test"; class Subcontext { public: - Subcontext(std::vector path_prefixes, std::string_view context, bool host = false) + Subcontext(std::vector path_prefixes, std::vector partitions, + std::string_view context, bool host = false) : path_prefixes_(std::move(path_prefixes)), + partitions_(std::move(partitions)), context_(context.begin(), context.end()), pid_(0) { if (!host) { @@ -49,6 +51,7 @@ class Subcontext { Result> ExpandArgs(const std::vector& args); void Restart(); bool PathMatchesSubcontext(const std::string& path) const; + bool PartitionMatchesSubcontext(const std::string& partition) const; void SetApexList(std::vector&& apex_list); const std::string& context() const { return context_; } @@ -59,6 +62,7 @@ class Subcontext { Result TransmitMessage(const SubcontextCommand& subcontext_command); std::vector path_prefixes_; + std::vector partitions_; std::vector apex_list_; std::string context_; pid_t pid_; diff --git a/init/subcontext_benchmark.cpp b/init/subcontext_benchmark.cpp index ccef2f36a..172ee3173 100644 --- a/init/subcontext_benchmark.cpp +++ b/init/subcontext_benchmark.cpp @@ -33,7 +33,7 @@ static void BenchmarkSuccess(benchmark::State& state) { return; } - auto subcontext = Subcontext({"path"}, context); + auto subcontext = Subcontext({"path"}, {"partition"}, context); free(context); while (state.KeepRunning()) { diff --git a/init/subcontext_test.cpp b/init/subcontext_test.cpp index da1f45550..85a2f2a94 100644 --- a/init/subcontext_test.cpp +++ b/init/subcontext_test.cpp @@ -41,7 +41,7 @@ namespace init { template void RunTest(F&& test_function) { - auto subcontext = Subcontext({"dummy_path"}, kTestContext); + auto subcontext = Subcontext({"dummy_path"}, {"dummy_partition"}, kTestContext); ASSERT_NE(0, subcontext.pid()); test_function(subcontext); @@ -177,6 +177,19 @@ TEST(subcontext, ExpandArgsFailure) { }); } +TEST(subcontext, PartitionMatchesSubcontext) { + RunTest([](auto& subcontext) { + static auto& existent_partition = "dummy_partition"; + static auto& non_existent_partition = "not_dummy_partition"; + + auto existent_result = subcontext.PartitionMatchesSubcontext(existent_partition); + auto non_existent_result = subcontext.PartitionMatchesSubcontext(non_existent_partition); + + ASSERT_TRUE(existent_result); + ASSERT_FALSE(non_existent_result); + }); +} + BuiltinFunctionMap BuildTestFunctionMap() { // For CheckDifferentPid auto do_return_pids_as_error = [](const BuiltinArguments& args) -> Result {