Appops permission monitoring for GET_USAGE_STATS.
This makes sure DataLoader won't be able to obtain read logs once user
denies access.
Bug: b/152633648
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest
Test: adb shell appops set 1000 GET_USAGE_STATS deny
Change-Id: Ibbb74933b4ef0dd8f5fe27732743e5820b8ee4dc
diff --git a/PREUPLOAD.cfg b/PREUPLOAD.cfg
index 9abb308..2fd2e33 100644
--- a/PREUPLOAD.cfg
+++ b/PREUPLOAD.cfg
@@ -9,6 +9,7 @@
core/jni/
libs/input/
services/core/jni/
+ services/incremental/
[Hook Scripts]
checkstyle_hook = ${REPO_ROOT}/prebuilts/checkstyle/checkstyle.py --sha ${PREUPLOAD_COMMIT}
diff --git a/services/incremental/Android.bp b/services/incremental/Android.bp
index 02bb0bc..b13d330 100644
--- a/services/incremental/Android.bp
+++ b/services/incremental/Android.bp
@@ -50,7 +50,6 @@
"libbinder",
"libcrypto",
"libcutils",
- "libdataloader",
"libincfs",
"liblog",
"libz",
diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 0da1673..d36eae8 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -17,7 +17,6 @@
#define LOG_TAG "IncrementalService"
#include "IncrementalService.h"
-#include "IncrementalServiceValidation.h"
#include <android-base/file.h>
#include <android-base/logging.h>
@@ -582,25 +581,29 @@
return -EINVAL;
}
- ifs->dataLoaderFilesystemParams.readLogsEnabled = enableReadLogs;
if (enableReadLogs) {
- // We never unregister the callbacks, but given a restricted number of data loaders and even fewer asking for read log access, should be ok.
- registerAppOpsCallback(ifs->dataLoaderParams.packageName);
- }
-
- return applyStorageParams(*ifs);
-}
-
-int IncrementalService::applyStorageParams(IncFsMount& ifs) {
- const bool enableReadLogs = ifs.dataLoaderFilesystemParams.readLogsEnabled;
- if (enableReadLogs) {
- if (auto status = CheckPermissionForDataDelivery(kDataUsageStats, kOpUsage);
+ if (auto status =
+ mAppOpsManager->checkPermission(kDataUsageStats, kOpUsage,
+ ifs->dataLoaderParams.packageName.c_str());
!status.isOk()) {
- LOG(ERROR) << "CheckPermissionForDataDelivery failed: " << status.toString8();
+ LOG(ERROR) << "checkPermission failed: " << status.toString8();
return fromBinderStatus(status);
}
}
+ if (auto status = applyStorageParams(*ifs, enableReadLogs); !status.isOk()) {
+ LOG(ERROR) << "applyStorageParams failed: " << status.toString8();
+ return fromBinderStatus(status);
+ }
+
+ if (enableReadLogs) {
+ registerAppOpsCallback(ifs->dataLoaderParams.packageName);
+ }
+
+ return 0;
+}
+
+binder::Status IncrementalService::applyStorageParams(IncFsMount& ifs, bool enableReadLogs) {
using unique_fd = ::android::base::unique_fd;
::android::os::incremental::IncrementalFileSystemControlParcel control;
control.cmd.reset(unique_fd(dup(ifs.control.cmd())));
@@ -611,13 +614,7 @@
}
std::lock_guard l(mMountOperationLock);
- const auto status = mVold->setIncFsMountOptions(control, enableReadLogs);
- if (!status.isOk()) {
- LOG(ERROR) << "Calling Vold::setIncFsMountOptions() failed: " << status.toString8();
- return fromBinderStatus(status);
- }
-
- return 0;
+ return mVold->setIncFsMountOptions(control, enableReadLogs);
}
void IncrementalService::deleteStorage(StorageId storageId) {
@@ -1280,39 +1277,54 @@
}
void IncrementalService::registerAppOpsCallback(const std::string& packageName) {
- if (packageName.empty()) {
+ sp<IAppOpsCallback> listener;
+ {
+ std::unique_lock lock{mCallbacksLock};
+ auto& cb = mCallbackRegistered[packageName];
+ if (cb) {
+ return;
+ }
+ cb = new AppOpsListener(*this, packageName);
+ listener = cb;
+ }
+
+ mAppOpsManager->startWatchingMode(AppOpsManager::OP_GET_USAGE_STATS, String16(packageName.c_str()), listener);
+}
+
+bool IncrementalService::unregisterAppOpsCallback(const std::string& packageName) {
+ sp<IAppOpsCallback> listener;
+ {
+ std::unique_lock lock{mCallbacksLock};
+ auto found = mCallbackRegistered.find(packageName);
+ if (found == mCallbackRegistered.end()) {
+ return false;
+ }
+ listener = found->second;
+ mCallbackRegistered.erase(found);
+ }
+
+ mAppOpsManager->stopWatchingMode(listener);
+ return true;
+}
+
+void IncrementalService::onAppOpChanged(const std::string& packageName) {
+ if (!unregisterAppOpsCallback(packageName)) {
return;
}
- {
- std::unique_lock lock{mCallbacksLock};
- if (!mCallbackRegistered.insert(packageName).second) {
- return;
- }
- }
-
- /* TODO(b/152633648): restore callback after it's not crashing Binder anymore.
- sp<AppOpsListener> listener = new AppOpsListener(*this, packageName);
- mAppOpsManager->startWatchingMode(AppOpsManager::OP_GET_USAGE_STATS, String16(packageName.c_str()), listener);
- */
-}
-
-void IncrementalService::onAppOppChanged(const std::string& packageName) {
std::vector<IfsMountPtr> affected;
{
std::lock_guard l(mLock);
affected.reserve(mMounts.size());
for (auto&& [id, ifs] : mMounts) {
- if (ifs->dataLoaderFilesystemParams.readLogsEnabled && ifs->dataLoaderParams.packageName == packageName) {
+ if (ifs->mountId == id && ifs->dataLoaderParams.packageName == packageName) {
affected.push_back(ifs);
}
}
}
- /* TODO(b/152633648): restore callback after it's not crashing Kernel anymore.
for (auto&& ifs : affected) {
- applyStorageParams(*ifs);
+ applyStorageParams(*ifs, false);
}
- */
}
binder::Status IncrementalService::IncrementalDataLoaderListener::onStatusChanged(MountId mountId,
@@ -1378,8 +1390,8 @@
return binder::Status::ok();
}
-void IncrementalService::AppOpsListener::opChanged(int32_t op, const String16&) {
- incrementalService.onAppOppChanged(packageName);
+void IncrementalService::AppOpsListener::opChanged(int32_t, const String16&) {
+ incrementalService.onAppOpChanged(packageName);
}
} // namespace android::incremental
diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index ff69633..5800297 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -40,7 +40,6 @@
#include "ServiceWrappers.h"
#include "android/content/pm/BnDataLoaderStatusListener.h"
#include "incfs.h"
-#include "dataloader_ndk.h"
#include "path.h"
using namespace android::os::incremental;
@@ -182,7 +181,6 @@
StorageMap storages;
BindMap bindPoints;
DataLoaderParamsParcel dataLoaderParams;
- DataLoaderFilesystemParams dataLoaderFilesystemParams;
std::atomic<int> nextStorageDirNo{0};
std::atomic<int> dataLoaderStatus = -1;
bool dataLoaderStartRequested = false;
@@ -193,9 +191,7 @@
: root(std::move(root)),
control(std::move(control)),
mountId(mountId),
- incrementalService(incrementalService) {
- dataLoaderFilesystemParams.readLogsEnabled = false;
- }
+ incrementalService(incrementalService) {}
IncFsMount(IncFsMount&&) = delete;
IncFsMount& operator=(IncFsMount&&) = delete;
~IncFsMount();
@@ -234,10 +230,11 @@
std::string normalizePathToStorage(const IfsMountPtr incfs, StorageId storage,
std::string_view path);
- int applyStorageParams(IncFsMount& ifs);
+ binder::Status applyStorageParams(IncFsMount& ifs, bool enableReadLogs);
void registerAppOpsCallback(const std::string& packageName);
- void onAppOppChanged(const std::string& packageName);
+ bool unregisterAppOpsCallback(const std::string& packageName);
+ void onAppOpChanged(const std::string& packageName);
// Member variables
std::unique_ptr<VoldServiceWrapper> const mVold;
@@ -252,7 +249,7 @@
BindPathMap mBindsByPath;
std::mutex mCallbacksLock;
- std::set<std::string> mCallbackRegistered;
+ std::map<std::string, sp<AppOpsListener>> mCallbackRegistered;
std::atomic_bool mSystemReady = false;
StorageId mNextId = 0;
diff --git a/services/incremental/IncrementalServiceValidation.h b/services/incremental/IncrementalServiceValidation.h
index 24f9f7f..48894c6 100644
--- a/services/incremental/IncrementalServiceValidation.h
+++ b/services/incremental/IncrementalServiceValidation.h
@@ -41,7 +41,8 @@
: -EIO;
}
-inline binder::Status CheckPermissionForDataDelivery(const char* permission, const char* operation) {
+inline binder::Status CheckPermissionForDataDelivery(const char* permission, const char* operation,
+ const char* package) {
using android::base::StringPrintf;
int32_t pid;
@@ -52,23 +53,23 @@
StringPrintf("UID %d / PID %d lacks permission %s", uid, pid, permission));
}
+ String16 packageName{package};
+
// Caller must also have op granted.
PermissionController pc;
- // Package is a required parameter. Need to obtain one.
- Vector<String16> packages;
- pc.getPackagesForUid(uid, packages);
- if (packages.empty()) {
+ if (auto packageUid = pc.getPackageUid(packageName, 0); packageUid != uid) {
return Exception(binder::Status::EX_SECURITY,
- StringPrintf("UID %d / PID %d has no packages", uid, pid));
+ StringPrintf("UID %d / PID %d does not own package %s", uid, pid,
+ package));
}
- switch (auto result = pc.noteOp(String16(operation), uid, packages[0]); result) {
+ switch (auto result = pc.noteOp(String16(operation), uid, packageName); result) {
case PermissionController::MODE_ALLOWED:
case PermissionController::MODE_DEFAULT:
return binder::Status::ok();
default:
return Exception(binder::Status::EX_SECURITY,
- StringPrintf("UID %d / PID %d lacks app-op %s, error %d", uid, pid,
- operation, result));
+ StringPrintf("UID %d / PID %d / package %s lacks app-op %s, error %d",
+ uid, pid, package, operation, result));
}
}
diff --git a/services/incremental/ServiceWrappers.h b/services/incremental/ServiceWrappers.h
index 449b457..84bf1ff 100644
--- a/services/incremental/ServiceWrappers.h
+++ b/services/incremental/ServiceWrappers.h
@@ -16,6 +16,8 @@
#pragma once
+#include "IncrementalServiceValidation.h"
+
#include <android-base/strings.h>
#include <android-base/unique_fd.h>
#include <android/content/pm/DataLoaderParamsParcel.h>
@@ -85,7 +87,10 @@
class AppOpsManagerWrapper {
public:
virtual ~AppOpsManagerWrapper() = default;
+ virtual binder::Status checkPermission(const char* permission, const char* operation,
+ const char* package) const = 0;
virtual void startWatchingMode(int32_t op, const String16& packageName, const sp<IAppOpsCallback>& callback) = 0;
+ virtual void stopWatchingMode(const sp<IAppOpsCallback>& callback) = 0;
};
class ServiceManagerWrapper {
@@ -105,17 +110,19 @@
~RealVoldService() = default;
binder::Status mountIncFs(const std::string& backingPath, const std::string& targetDir,
int32_t flags,
- IncrementalFileSystemControlParcel* _aidl_return) const override {
+ IncrementalFileSystemControlParcel* _aidl_return) const final {
return mInterface->mountIncFs(backingPath, targetDir, flags, _aidl_return);
}
- binder::Status unmountIncFs(const std::string& dir) const override {
+ binder::Status unmountIncFs(const std::string& dir) const final {
return mInterface->unmountIncFs(dir);
}
binder::Status bindMount(const std::string& sourceDir,
- const std::string& targetDir) const override {
+ const std::string& targetDir) const final {
return mInterface->bindMount(sourceDir, targetDir);
}
- binder::Status setIncFsMountOptions(const ::android::os::incremental::IncrementalFileSystemControlParcel& control, bool enableReadLogs) const override {
+ binder::Status setIncFsMountOptions(
+ const ::android::os::incremental::IncrementalFileSystemControlParcel& control,
+ bool enableReadLogs) const final {
return mInterface->setIncFsMountOptions(control, enableReadLogs);
}
@@ -131,13 +138,13 @@
binder::Status initializeDataLoader(MountId mountId, const DataLoaderParamsParcel& params,
const FileSystemControlParcel& control,
const sp<IDataLoaderStatusListener>& listener,
- bool* _aidl_return) const override {
+ bool* _aidl_return) const final {
return mInterface->initializeDataLoader(mountId, params, control, listener, _aidl_return);
}
- binder::Status getDataLoader(MountId mountId, sp<IDataLoader>* _aidl_return) const override {
+ binder::Status getDataLoader(MountId mountId, sp<IDataLoader>* _aidl_return) const final {
return mInterface->getDataLoader(mountId, _aidl_return);
}
- binder::Status destroyDataLoader(MountId mountId) const override {
+ binder::Status destroyDataLoader(MountId mountId) const final {
return mInterface->destroyDataLoader(mountId);
}
@@ -148,9 +155,18 @@
class RealAppOpsManager : public AppOpsManagerWrapper {
public:
~RealAppOpsManager() = default;
- void startWatchingMode(int32_t op, const String16& packageName, const sp<IAppOpsCallback>& callback) override {
+ binder::Status checkPermission(const char* permission, const char* operation,
+ const char* package) const final {
+ return android::incremental::CheckPermissionForDataDelivery(permission, operation, package);
+ }
+ void startWatchingMode(int32_t op, const String16& packageName,
+ const sp<IAppOpsCallback>& callback) final {
mAppOpsManager.startWatchingMode(op, packageName, callback);
}
+ void stopWatchingMode(const sp<IAppOpsCallback>& callback) final {
+ mAppOpsManager.stopWatchingMode(callback);
+ }
+
private:
android::AppOpsManager mAppOpsManager;
};
@@ -174,36 +190,35 @@
public:
RealIncFs() = default;
~RealIncFs() = default;
- Control createControl(IncFsFd cmd, IncFsFd pendingReads, IncFsFd logs) const override {
+ Control createControl(IncFsFd cmd, IncFsFd pendingReads, IncFsFd logs) const final {
return incfs::createControl(cmd, pendingReads, logs);
}
ErrorCode makeFile(const Control& control, std::string_view path, int mode, FileId id,
- NewFileParams params) const override {
+ NewFileParams params) const final {
return incfs::makeFile(control, path, mode, id, params);
}
- ErrorCode makeDir(const Control& control, std::string_view path, int mode) const override {
+ ErrorCode makeDir(const Control& control, std::string_view path, int mode) const final {
return incfs::makeDir(control, path, mode);
}
- RawMetadata getMetadata(const Control& control, FileId fileid) const override {
+ RawMetadata getMetadata(const Control& control, FileId fileid) const final {
return incfs::getMetadata(control, fileid);
}
- RawMetadata getMetadata(const Control& control, std::string_view path) const override {
+ RawMetadata getMetadata(const Control& control, std::string_view path) const final {
return incfs::getMetadata(control, path);
}
- FileId getFileId(const Control& control, std::string_view path) const override {
+ FileId getFileId(const Control& control, std::string_view path) const final {
return incfs::getFileId(control, path);
}
- ErrorCode link(const Control& control, std::string_view from,
- std::string_view to) const override {
+ ErrorCode link(const Control& control, std::string_view from, std::string_view to) const final {
return incfs::link(control, from, to);
}
- ErrorCode unlink(const Control& control, std::string_view path) const override {
+ ErrorCode unlink(const Control& control, std::string_view path) const final {
return incfs::unlink(control, path);
}
- base::unique_fd openForSpecialOps(const Control& control, FileId id) const override {
+ base::unique_fd openForSpecialOps(const Control& control, FileId id) const final {
return base::unique_fd{incfs::openForSpecialOps(control, id).release()};
}
- ErrorCode writeBlocks(Span<const DataBlock> blocks) const override {
+ ErrorCode writeBlocks(Span<const DataBlock> blocks) const final {
return incfs::writeBlocks(blocks);
}
};
diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp
index 5553f68..0635ae1 100644
--- a/services/incremental/test/IncrementalServiceTest.cpp
+++ b/services/incremental/test/IncrementalServiceTest.cpp
@@ -221,7 +221,28 @@
};
class MockAppOpsManager : public AppOpsManagerWrapper {
+public:
+ MOCK_CONST_METHOD3(checkPermission, binder::Status(const char*, const char*, const char*));
MOCK_METHOD3(startWatchingMode, void(int32_t, const String16&, const sp<IAppOpsCallback>&));
+ MOCK_METHOD1(stopWatchingMode, void(const sp<IAppOpsCallback>&));
+
+ void checkPermissionSuccess() {
+ ON_CALL(*this, checkPermission(_, _, _)).WillByDefault(Return(android::incremental::Ok()));
+ }
+ void checkPermissionFails() {
+ ON_CALL(*this, checkPermission(_, _, _))
+ .WillByDefault(
+ Return(android::incremental::Exception(binder::Status::EX_SECURITY, {})));
+ }
+ void initializeStartWatchingMode() {
+ ON_CALL(*this, startWatchingMode(_, _, _))
+ .WillByDefault(Invoke(this, &MockAppOpsManager::storeCallback));
+ }
+ void storeCallback(int32_t, const String16&, const sp<IAppOpsCallback>& cb) {
+ mStoredCallback = cb;
+ }
+
+ sp<IAppOpsCallback> mStoredCallback;
};
class MockServiceManager : public ServiceManagerWrapper {
@@ -418,9 +439,15 @@
mVold->setIncFsMountOptionsSuccess();
mDataLoaderManager->initializeDataLoaderSuccess();
mDataLoaderManager->getDataLoaderSuccess();
+ mAppOpsManager->checkPermissionSuccess();
EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
- EXPECT_CALL(*mVold, setIncFsMountOptions(_, _));
+ // We are calling setIncFsMountOptions(true).
+ EXPECT_CALL(*mVold, setIncFsMountOptions(_, true)).Times(1);
+ // After setIncFsMountOptions succeeded expecting to start watching.
+ EXPECT_CALL(*mAppOpsManager, startWatchingMode(_, _, _)).Times(1);
+ // Not expecting callback removal.
+ EXPECT_CALL(*mAppOpsManager, stopWatchingMode(_)).Times(0);
TemporaryDir tempDir;
int storageId =
mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},
@@ -429,6 +456,56 @@
ASSERT_GE(mIncrementalService->setStorageParams(storageId, true), 0);
}
+TEST_F(IncrementalServiceTest, testSetIncFsMountOptionsSuccessAndPermissionChanged) {
+ mVold->mountIncFsSuccess();
+ mIncFs->makeFileSuccess();
+ mVold->bindMountSuccess();
+ mVold->setIncFsMountOptionsSuccess();
+ mDataLoaderManager->initializeDataLoaderSuccess();
+ mDataLoaderManager->getDataLoaderSuccess();
+ mAppOpsManager->checkPermissionSuccess();
+ mAppOpsManager->initializeStartWatchingMode();
+ EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
+ EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
+ // We are calling setIncFsMountOptions(true).
+ EXPECT_CALL(*mVold, setIncFsMountOptions(_, true)).Times(1);
+ // setIncFsMountOptions(false) is called on the callback.
+ EXPECT_CALL(*mVold, setIncFsMountOptions(_, false)).Times(1);
+ // After setIncFsMountOptions succeeded expecting to start watching.
+ EXPECT_CALL(*mAppOpsManager, startWatchingMode(_, _, _)).Times(1);
+ // After callback is called, disable read logs and remove callback.
+ EXPECT_CALL(*mAppOpsManager, stopWatchingMode(_)).Times(1);
+ TemporaryDir tempDir;
+ int storageId =
+ mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},
+ IncrementalService::CreateOptions::CreateNew);
+ ASSERT_GE(storageId, 0);
+ ASSERT_GE(mIncrementalService->setStorageParams(storageId, true), 0);
+ ASSERT_NE(nullptr, mAppOpsManager->mStoredCallback.get());
+ mAppOpsManager->mStoredCallback->opChanged(0, {});
+}
+
+TEST_F(IncrementalServiceTest, testSetIncFsMountOptionsCheckPermissionFails) {
+ mVold->mountIncFsSuccess();
+ mIncFs->makeFileSuccess();
+ mVold->bindMountSuccess();
+ mDataLoaderManager->initializeDataLoaderSuccess();
+ mDataLoaderManager->getDataLoaderSuccess();
+ mAppOpsManager->checkPermissionFails();
+ EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
+ EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
+ // checkPermission fails, no calls to set opitions, start or stop WatchingMode.
+ EXPECT_CALL(*mVold, setIncFsMountOptions(_, true)).Times(0);
+ EXPECT_CALL(*mAppOpsManager, startWatchingMode(_, _, _)).Times(0);
+ EXPECT_CALL(*mAppOpsManager, stopWatchingMode(_)).Times(0);
+ TemporaryDir tempDir;
+ int storageId =
+ mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},
+ IncrementalService::CreateOptions::CreateNew);
+ ASSERT_GE(storageId, 0);
+ ASSERT_LT(mIncrementalService->setStorageParams(storageId, true), 0);
+}
+
TEST_F(IncrementalServiceTest, testSetIncFsMountOptionsFails) {
mVold->mountIncFsSuccess();
mIncFs->makeFileSuccess();
@@ -436,9 +513,14 @@
mVold->setIncFsMountOptionsFails();
mDataLoaderManager->initializeDataLoaderSuccess();
mDataLoaderManager->getDataLoaderSuccess();
+ mAppOpsManager->checkPermissionSuccess();
EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
- EXPECT_CALL(*mVold, setIncFsMountOptions(_, _));
+ // We are calling setIncFsMountOptions.
+ EXPECT_CALL(*mVold, setIncFsMountOptions(_, true)).Times(1);
+ // setIncFsMountOptions fails, no calls to start or stop WatchingMode.
+ EXPECT_CALL(*mAppOpsManager, startWatchingMode(_, _, _)).Times(0);
+ EXPECT_CALL(*mAppOpsManager, stopWatchingMode(_)).Times(0);
TemporaryDir tempDir;
int storageId =
mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},