transcoding: move uid/pid from client to job
Remove the uid/pid on registerClient, and move uid/pid
checking to submitRequest time.
bug: 154733526
test: unit testing, builds
Change-Id: Ibf29e326cd80e133b8630df96382c80dd9002548
diff --git a/media/libmediatranscoding/TranscodingClientManager.cpp b/media/libmediatranscoding/TranscodingClientManager.cpp
index ce3ac13..d9f3f28 100644
--- a/media/libmediatranscoding/TranscodingClientManager.cpp
+++ b/media/libmediatranscoding/TranscodingClientManager.cpp
@@ -23,6 +23,7 @@
#include <inttypes.h>
#include <media/TranscodingClientManager.h>
#include <media/TranscodingRequest.h>
+#include <private/android_filesystem_config.h>
#include <utils/Log.h>
namespace android {
@@ -44,6 +45,26 @@
TranscodingClientManager::sCookie2Client;
///////////////////////////////////////////////////////////////////////////////
+// Convenience methods for constructing binder::Status objects for error returns
+#define STATUS_ERROR_FMT(errorCode, errorString, ...) \
+ Status::fromServiceSpecificErrorWithMessage( \
+ errorCode, \
+ String8::format("%s:%d: " errorString, __FUNCTION__, __LINE__, ##__VA_ARGS__))
+
+// Can MediaTranscoding service trust the caller based on the calling UID?
+// TODO(hkuang): Add MediaProvider's UID.
+static bool isTrustedCallingUid(uid_t uid) {
+ switch (uid) {
+ case AID_ROOT: // root user
+ case AID_SYSTEM:
+ case AID_SHELL:
+ case AID_MEDIA: // mediaserver
+ return true;
+ default:
+ return false;
+ }
+}
+
/**
* ClientImpl implements a single client and contains all its information.
*/
@@ -60,8 +81,6 @@
* (casted to int64t_t) as the client id.
*/
ClientIdType mClientId;
- pid_t mClientPid;
- uid_t mClientUid;
std::string mClientName;
std::string mClientOpPackageName;
@@ -72,7 +91,7 @@
// Weak pointer to the client manager for this client.
std::weak_ptr<TranscodingClientManager> mOwner;
- ClientImpl(const std::shared_ptr<ITranscodingClientCallback>& callback, pid_t pid, uid_t uid,
+ ClientImpl(const std::shared_ptr<ITranscodingClientCallback>& callback,
const std::string& clientName, const std::string& opPackageName,
const std::weak_ptr<TranscodingClientManager>& owner);
@@ -88,14 +107,11 @@
};
TranscodingClientManager::ClientImpl::ClientImpl(
- const std::shared_ptr<ITranscodingClientCallback>& callback, pid_t pid, uid_t uid,
- const std::string& clientName, const std::string& opPackageName,
- const std::weak_ptr<TranscodingClientManager>& owner)
+ const std::shared_ptr<ITranscodingClientCallback>& callback, const std::string& clientName,
+ const std::string& opPackageName, const std::weak_ptr<TranscodingClientManager>& owner)
: mClientBinder((callback != nullptr) ? callback->asBinder() : nullptr),
mClientCallback(callback),
mClientId(sCookieCounter.fetch_add(1, std::memory_order_relaxed)),
- mClientPid(pid),
- mClientUid(uid),
mClientName(clientName),
mClientOpPackageName(opPackageName),
mNextJobId(0),
@@ -113,14 +129,52 @@
}
if (in_request.sourceFilePath.empty() || in_request.destinationFilePath.empty()) {
- // This is the only error we check for now.
return Status::ok();
}
+ int32_t callingPid = AIBinder_getCallingPid();
+ int32_t callingUid = AIBinder_getCallingUid();
+ int32_t in_clientUid = in_request.clientUid;
+ int32_t in_clientPid = in_request.clientPid;
+
+ // Check if we can trust clientUid. Only privilege caller could forward the
+ // uid on app client's behalf.
+ if (in_clientUid == IMediaTranscodingService::USE_CALLING_UID) {
+ in_clientUid = callingUid;
+ } else if (in_clientUid < 0) {
+ return Status::ok();
+ } else if (in_clientUid != callingUid && !isTrustedCallingUid(callingUid)) {
+ ALOGE("MediaTranscodingService::registerClient rejected (clientPid %d, clientUid %d) "
+ "(don't trust callingUid %d)",
+ in_clientPid, in_clientUid, callingUid);
+ return STATUS_ERROR_FMT(
+ IMediaTranscodingService::ERROR_PERMISSION_DENIED,
+ "MediaTranscodingService::registerClient rejected (clientPid %d, clientUid %d) "
+ "(don't trust callingUid %d)",
+ in_clientPid, in_clientUid, callingUid);
+ }
+
+ // Check if we can trust clientPid. Only privilege caller could forward the
+ // pid on app client's behalf.
+ if (in_clientPid == IMediaTranscodingService::USE_CALLING_PID) {
+ in_clientPid = callingPid;
+ } else if (in_clientPid < 0) {
+ return Status::ok();
+ } else if (in_clientPid != callingPid && !isTrustedCallingUid(callingUid)) {
+ ALOGE("MediaTranscodingService::registerClient rejected (clientPid %d, clientUid %d) "
+ "(don't trust callingUid %d)",
+ in_clientPid, in_clientUid, callingUid);
+ return STATUS_ERROR_FMT(
+ IMediaTranscodingService::ERROR_PERMISSION_DENIED,
+ "MediaTranscodingService::registerClient rejected (clientPid %d, clientUid %d) "
+ "(don't trust callingUid %d)",
+ in_clientPid, in_clientUid, callingUid);
+ }
+
int32_t jobId = mNextJobId.fetch_add(1);
- *_aidl_return =
- owner->mJobScheduler->submit(mClientId, jobId, mClientUid, in_request, mClientCallback);
+ *_aidl_return = owner->mJobScheduler->submit(mClientId, jobId, in_clientUid, in_request,
+ mClientCallback);
if (*_aidl_return) {
out_job->jobId = jobId;
@@ -246,11 +300,10 @@
}
status_t TranscodingClientManager::addClient(
- const std::shared_ptr<ITranscodingClientCallback>& callback, pid_t pid, uid_t uid,
- const std::string& clientName, const std::string& opPackageName,
- std::shared_ptr<ITranscodingClient>* outClient) {
+ const std::shared_ptr<ITranscodingClientCallback>& callback, const std::string& clientName,
+ const std::string& opPackageName, std::shared_ptr<ITranscodingClient>* outClient) {
// Validate the client.
- if (callback == nullptr || pid < 0 || clientName.empty() || opPackageName.empty()) {
+ if (callback == nullptr || clientName.empty() || opPackageName.empty()) {
ALOGE("Invalid client");
return IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT;
}
@@ -264,12 +317,11 @@
return IMediaTranscodingService::ERROR_ALREADY_EXISTS;
}
- // Creates the client and uses its process id as client id.
+ // Creates the client (with the id assigned by ClientImpl).
std::shared_ptr<ClientImpl> client = ::ndk::SharedRefBase::make<ClientImpl>(
- callback, pid, uid, clientName, opPackageName, shared_from_this());
+ callback, clientName, opPackageName, shared_from_this());
- ALOGD("Adding client id %lld, pid %d, uid %d, name %s, package %s",
- (long long)client->mClientId, client->mClientPid, client->mClientUid,
+ ALOGD("Adding client id %lld, name %s, package %s", (long long)client->mClientId,
client->mClientName.c_str(), client->mClientOpPackageName.c_str());
{
diff --git a/media/libmediatranscoding/aidl/android/media/IMediaTranscodingService.aidl b/media/libmediatranscoding/aidl/android/media/IMediaTranscodingService.aidl
index 40ca2c2..7fc7748 100644
--- a/media/libmediatranscoding/aidl/android/media/IMediaTranscodingService.aidl
+++ b/media/libmediatranscoding/aidl/android/media/IMediaTranscodingService.aidl
@@ -58,17 +58,13 @@
* the client.
* @param clientName name of the client.
* @param opPackageName op package name of the client.
- * @param clientUid user id of the client.
- * @param clientPid process id of the client.
* @return an ITranscodingClient interface object, with nullptr indicating
* failure to register.
*/
ITranscodingClient registerClient(
in ITranscodingClientCallback callback,
in String clientName,
- in String opPackageName,
- in int clientUid,
- in int clientPid);
+ in String opPackageName);
/**
* Returns the number of clients. This is used for debugging.
diff --git a/media/libmediatranscoding/include/media/TranscodingClientManager.h b/media/libmediatranscoding/include/media/TranscodingClientManager.h
index a62ad8c..015a83a 100644
--- a/media/libmediatranscoding/include/media/TranscodingClientManager.h
+++ b/media/libmediatranscoding/include/media/TranscodingClientManager.h
@@ -58,16 +58,14 @@
* already been added, it will also return non-zero errorcode.
*
* @param callback client callback for the service to call this client.
- * @param pid client's process id.
- * @param uid client's user id.
* @param clientName client's name.
* @param opPackageName client's package name.
* @param client output holding the ITranscodingClient interface for the client
* to use for subsequent communications with the service.
* @return 0 if client is added successfully, non-zero errorcode otherwise.
*/
- status_t addClient(const std::shared_ptr<ITranscodingClientCallback>& callback, pid_t pid,
- uid_t uid, const std::string& clientName, const std::string& opPackageName,
+ status_t addClient(const std::shared_ptr<ITranscodingClientCallback>& callback,
+ const std::string& clientName, const std::string& opPackageName,
std::shared_ptr<ITranscodingClient>* client);
/**
diff --git a/media/libmediatranscoding/tests/TranscodingClientManager_tests.cpp b/media/libmediatranscoding/tests/TranscodingClientManager_tests.cpp
index 1583325..41f3ada 100644
--- a/media/libmediatranscoding/tests/TranscodingClientManager_tests.cpp
+++ b/media/libmediatranscoding/tests/TranscodingClientManager_tests.cpp
@@ -43,12 +43,11 @@
using ::aidl::android::media::TranscodingRequestParcel;
using ::aidl::android::media::TranscodingResultParcel;
-constexpr pid_t kInvalidClientPid = -1;
+constexpr pid_t kInvalidClientPid = -5;
+constexpr pid_t kInvalidClientUid = -10;
constexpr const char* kInvalidClientName = "";
constexpr const char* kInvalidClientPackage = "";
-constexpr pid_t kClientPid = 2;
-constexpr uid_t kClientUid = 3;
constexpr const char* kClientName = "TestClientName";
constexpr const char* kClientPackage = "TestClientPackage";
@@ -236,17 +235,17 @@
~TranscodingClientManagerTest() { ALOGD("TranscodingClientManagerTest destroyed"); }
void addMultipleClients() {
- EXPECT_EQ(mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, kClientName,
+ EXPECT_EQ(mClientManager->addClient(mClientCallback1, kClientName,
kClientPackage, &mClient1),
OK);
EXPECT_NE(mClient1, nullptr);
- EXPECT_EQ(mClientManager->addClient(mClientCallback2, kClientPid, kClientUid, kClientName,
+ EXPECT_EQ(mClientManager->addClient(mClientCallback2, kClientName,
kClientPackage, &mClient2),
OK);
EXPECT_NE(mClient2, nullptr);
- EXPECT_EQ(mClientManager->addClient(mClientCallback3, kClientPid, kClientUid, kClientName,
+ EXPECT_EQ(mClientManager->addClient(mClientCallback3, kClientName,
kClientPackage, &mClient3),
OK);
EXPECT_NE(mClient3, nullptr);
@@ -274,23 +273,23 @@
TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientCallback) {
// Add a client with null callback and expect failure.
std::shared_ptr<ITranscodingClient> client;
- status_t err = mClientManager->addClient(nullptr, kClientPid, kClientUid, kClientName,
+ status_t err = mClientManager->addClient(nullptr, kClientName,
kClientPackage, &client);
EXPECT_EQ(err, IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT);
}
-
-TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientPid) {
- // Add a client with invalid Pid and expect failure.
- std::shared_ptr<ITranscodingClient> client;
- status_t err = mClientManager->addClient(mClientCallback1, kInvalidClientPid, kClientUid,
- kClientName, kClientPackage, &client);
- EXPECT_EQ(err, IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT);
-}
+//
+//TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientPid) {
+// // Add a client with invalid Pid and expect failure.
+// std::shared_ptr<ITranscodingClient> client;
+// status_t err = mClientManager->addClient(mClientCallback1,
+// kClientName, kClientPackage, &client);
+// EXPECT_EQ(err, IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT);
+//}
TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientName) {
// Add a client with invalid name and expect failure.
std::shared_ptr<ITranscodingClient> client;
- status_t err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid,
+ status_t err = mClientManager->addClient(mClientCallback1,
kInvalidClientName, kClientPackage, &client);
EXPECT_EQ(err, IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT);
}
@@ -298,7 +297,7 @@
TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientPackageName) {
// Add a client with invalid packagename and expect failure.
std::shared_ptr<ITranscodingClient> client;
- status_t err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, kClientName,
+ status_t err = mClientManager->addClient(mClientCallback1, kClientName,
kInvalidClientPackage, &client);
EXPECT_EQ(err, IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT);
}
@@ -306,7 +305,7 @@
TEST_F(TranscodingClientManagerTest, TestAddingValidClient) {
// Add a valid client, should succeed.
std::shared_ptr<ITranscodingClient> client;
- status_t err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, kClientName,
+ status_t err = mClientManager->addClient(mClientCallback1, kClientName,
kClientPackage, &client);
EXPECT_EQ(err, OK);
EXPECT_NE(client.get(), nullptr);
@@ -320,14 +319,14 @@
TEST_F(TranscodingClientManagerTest, TestAddingDupliacteClient) {
std::shared_ptr<ITranscodingClient> client;
- status_t err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, kClientName,
+ status_t err = mClientManager->addClient(mClientCallback1, kClientName,
kClientPackage, &client);
EXPECT_EQ(err, OK);
EXPECT_NE(client.get(), nullptr);
EXPECT_EQ(mClientManager->getNumOfClients(), 1);
std::shared_ptr<ITranscodingClient> dupClient;
- err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, "dupClient",
+ err = mClientManager->addClient(mClientCallback1, "dupClient",
"dupPackage", &dupClient);
EXPECT_EQ(err, IMediaTranscodingService::ERROR_ALREADY_EXISTS);
EXPECT_EQ(dupClient.get(), nullptr);
@@ -337,8 +336,7 @@
EXPECT_TRUE(status.isOk());
EXPECT_EQ(mClientManager->getNumOfClients(), 0);
- err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, "dupClient",
- "dupPackage", &dupClient);
+ err = mClientManager->addClient(mClientCallback1, "dupClient", "dupPackage", &dupClient);
EXPECT_EQ(err, OK);
EXPECT_NE(dupClient.get(), nullptr);
EXPECT_EQ(mClientManager->getNumOfClients(), 1);
@@ -385,6 +383,14 @@
EXPECT_TRUE(mClient1->submitRequest(badRequest, &job, &result).isOk());
EXPECT_FALSE(result);
+ // Test submit with bad pid/uid.
+ badRequest.sourceFilePath = "test_source_file_3";
+ badRequest.destinationFilePath = "test_desintaion_file_3";
+ badRequest.clientPid = kInvalidClientPid;
+ badRequest.clientUid = kInvalidClientUid;
+ EXPECT_TRUE(mClient1->submitRequest(badRequest, &job, &result).isOk());
+ EXPECT_FALSE(result);
+
// Test get jobs by id.
EXPECT_TRUE(mClient1->getJobWithId(JOB(2), &job, &result).isOk());
EXPECT_EQ(job.jobId, JOB(2));
@@ -468,7 +474,7 @@
TEST_F(TranscodingClientManagerTest, TestUseAfterUnregister) {
// Add a client.
std::shared_ptr<ITranscodingClient> client;
- status_t err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, kClientName,
+ status_t err = mClientManager->addClient(mClientCallback1, kClientName,
kClientPackage, &client);
EXPECT_EQ(err, OK);
EXPECT_NE(client.get(), nullptr);