Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 3f23e986 authored by Chong Zhang's avatar Chong Zhang
Browse files

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
parent 2f0bf51f

media/libmediatranscoding/TranscodingClientManager.cpp

+71 −19
Original line number Diff line number Diff line
@@ -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 @@ std::map<ClientIdType, std::shared_ptr<TranscodingClientManager::ClientImpl>> 
        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 @@ struct TranscodingClientManager::ClientImpl : public BnTranscodingClient { 
     * (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 @@ struct TranscodingClientManager::ClientImpl : public BnTranscodingClient { 
    // 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 @@ struct TranscodingClientManager::ClientImpl : public BnTranscodingClient { 
};



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 @@ Status TranscodingClientManager::ClientImpl::submitRequest( 
    }



    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 @@ void TranscodingClientManager::dumpAllClients(int fd, const Vector<String16>& ar 
}



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 @@ status_t TranscodingClientManager::addClient( 
        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());



    {

media/libmediatranscoding/aidl/android/media/IMediaTranscodingService.aidl

+1 −5
Original line number Diff line number Diff line
@@ -58,17 +58,13 @@ interface IMediaTranscodingService { 
     *        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.

media/libmediatranscoding/include/media/TranscodingClientManager.h

+2 −4
Original line number Diff line number Diff line
@@ -58,16 +58,14 @@ public: 
     * 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);



    /**

media/libmediatranscoding/tests/TranscodingClientManager_tests.cpp

+29 −23
Original line number Diff line number Diff line
@@ -43,12 +43,11 @@ using ::aidl::android::media::TranscodingJobPriority; 
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 @@ public: 
    ~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 @@ public: 
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, TestAddingWithInvalidClientName) { 
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, TestAddingWithInvalidClientPackageName) { 
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, TestAddingValidClient) { 


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 @@ TEST_F(TranscodingClientManagerTest, TestAddingDupliacteClient) { 
    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 @@ TEST_F(TranscodingClientManagerTest, TestSubmitCancelGetJobs) { 
    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, TestClientCallback) { 
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);

services/mediatranscoding/MediaTranscodingService.cpp

+4 −52
Original line number Diff line number Diff line
@@ -27,7 +27,6 @@ 
#include <media/TranscodingJobScheduler.h>

#include <media/TranscodingResourcePolicy.h>

#include <media/TranscodingUidPolicy.h>

#include <private/android_filesystem_config.h>

#include <utils/Log.h>

#include <utils/Vector.h>
@@ -41,20 +40,6 @@ namespace android { 
            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;

    }

}



MediaTranscodingService::MediaTranscodingService(

        const std::shared_ptr<TranscoderInterface>& transcoder)

      : mUidPolicy(new TranscodingUidPolicy()),
@@ -116,51 +101,18 @@ void MediaTranscodingService::instantiate() { 


Status MediaTranscodingService::registerClient(

        const std::shared_ptr<ITranscodingClientCallback>& in_callback,

        const std::string& in_clientName, const std::string& in_opPackageName, int32_t in_clientUid,

        int32_t in_clientPid, std::shared_ptr<ITranscodingClient>* _aidl_return) {

        const std::string& in_clientName, const std::string& in_opPackageName,

        std::shared_ptr<ITranscodingClient>* _aidl_return) {

    if (in_callback == nullptr) {

        *_aidl_return = nullptr;

        return STATUS_ERROR_FMT(ERROR_ILLEGAL_ARGUMENT, "Client callback cannot be null!");

    }



    int32_t callingPid = AIBinder_getCallingPid();

    int32_t callingUid = AIBinder_getCallingUid();



    // Check if we can trust clientUid. Only privilege caller could forward the

    // uid on app client's behalf.

    if (in_clientUid == USE_CALLING_UID) {

        in_clientUid = callingUid;

    } else if (!isTrustedCallingUid(callingUid)) {

        ALOGE("MediaTranscodingService::registerClient failed (calling PID %d, calling UID %d) "

              "rejected "

              "(don't trust clientUid %d)",

              in_clientPid, in_clientUid, in_clientUid);

        return STATUS_ERROR_FMT(ERROR_PERMISSION_DENIED,

                                "Untrusted caller (calling PID %d, UID %d) trying to "

                                "register client",

                                in_clientPid, in_clientUid);

    }



    // Check if we can trust clientPid. Only privilege caller could forward the

    // pid on app client's behalf.

    if (in_clientPid == USE_CALLING_PID) {

        in_clientPid = callingPid;

    } else if (!isTrustedCallingUid(callingUid)) {

        ALOGE("MediaTranscodingService::registerClient client failed (calling PID %d, calling UID "

              "%d) rejected "

              "(don't trust clientPid %d)",

              in_clientPid, in_clientUid, in_clientPid);

        return STATUS_ERROR_FMT(ERROR_PERMISSION_DENIED,

                                "Untrusted caller (calling PID %d, UID %d) trying to "

                                "register client",

                                in_clientPid, in_clientUid);

    }



    // Creates the client and uses its process id as client id.

    std::shared_ptr<ITranscodingClient> newClient;



    status_t err = mClientManager->addClient(in_callback, in_clientPid, in_clientUid, in_clientName,

                                             in_opPackageName, &newClient);

    status_t err =

            mClientManager->addClient(in_callback, in_clientName, in_opPackageName, &newClient);

    if (err != OK) {

        *_aidl_return = nullptr;

        return STATUS_ERROR_FMT(err, "Failed to add client to TranscodingClientManager");