From d203eb6b3cd99af3c9616df2bd0d982796b0f918 Mon Sep 17 00:00:00 2001 From: Andy Hung Date: Mon, 27 Apr 2020 09:12:46 -0700 Subject: [PATCH] MediaMetrics: Use AllowUid property for services to control client access Remove implicit first client uid access. Test: adb shell dumpsys media.metrics Bug: 149850236 Change-Id: I4f953db4688bf5f174c3e1611f492f31d2926e96 --- .../include/MediaMetricsConstants.h | 1 + services/audioflinger/Tracks.cpp | 2 ++ services/mediametrics/TimeMachine.h | 29 ++++++++++----- .../mediametrics/tests/mediametrics_tests.cpp | 36 +++++++++++++++++++ .../oboeservice/AAudioServiceStreamBase.cpp | 1 + 5 files changed, 60 insertions(+), 9 deletions(-) diff --git a/media/libmediametrics/include/MediaMetricsConstants.h b/media/libmediametrics/include/MediaMetricsConstants.h index a7e79f4555..00da69aec8 100644 --- a/media/libmediametrics/include/MediaMetricsConstants.h +++ b/media/libmediametrics/include/MediaMetricsConstants.h @@ -86,6 +86,7 @@ // of suppressed in the Time Machine. #define AMEDIAMETRICS_PROP_SUFFIX_CHAR_DUPLICATES_ALLOWED '#' +#define AMEDIAMETRICS_PROP_ALLOWUID "_allowUid" // int32_t, allow client uid to post #define AMEDIAMETRICS_PROP_AUXEFFECTID "auxEffectId" // int32 (AudioTrack) #define AMEDIAMETRICS_PROP_BUFFERSIZEFRAMES "bufferSizeFrames" // int32 #define AMEDIAMETRICS_PROP_BUFFERCAPACITYFRAMES "bufferCapacityFrames" // int32 diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp index 23d8329316..4898d37998 100644 --- a/services/audioflinger/Tracks.cpp +++ b/services/audioflinger/Tracks.cpp @@ -605,6 +605,7 @@ AudioFlinger::PlaybackThread::Track::Track( mediametrics::LogItem(mMetricsId) .setPid(creatorPid) .setUid(uid) + .set(AMEDIAMETRICS_PROP_ALLOWUID, (int32_t)uid) .set(AMEDIAMETRICS_PROP_EVENT, AMEDIAMETRICS_PROP_PREFIX_SERVER AMEDIAMETRICS_PROP_EVENT_VALUE_CTOR) .record(); @@ -2165,6 +2166,7 @@ AudioFlinger::RecordThread::RecordTrack::RecordTrack( mediametrics::LogItem(mMetricsId) .setPid(creatorPid) .setUid(uid) + .set(AMEDIAMETRICS_PROP_ALLOWUID, (int32_t)uid) .set(AMEDIAMETRICS_PROP_EVENT, "server." AMEDIAMETRICS_PROP_EVENT_VALUE_CTOR) .record(); } diff --git a/services/mediametrics/TimeMachine.h b/services/mediametrics/TimeMachine.h index 6861c7847a..c82778b28f 100644 --- a/services/mediametrics/TimeMachine.h +++ b/services/mediametrics/TimeMachine.h @@ -75,21 +75,30 @@ private: class KeyHistory { public: template - KeyHistory(T key, pid_t pid, uid_t uid, int64_t time) + KeyHistory(T key, uid_t allowUid, int64_t time) : mKey(key) - , mPid(pid) - , mUid(uid) + , mAllowUid(allowUid) , mCreationTime(time) , mLastModificationTime(time) { - putValue(BUNDLE_PID, (int32_t)pid, time); - putValue(BUNDLE_UID, (int32_t)uid, time); + // allowUid allows an untrusted client with a matching uid to set properties + // in this key. + // If allowUid == (uid_t)-1, no untrusted client may set properties in the key. + if (allowUid != (uid_t)-1) { + // Set ALLOWUID property here; does not change after key creation. + putValue(AMEDIAMETRICS_PROP_ALLOWUID, (int32_t)allowUid, time); + } } KeyHistory(const KeyHistory &other) = default; + // Return NO_ERROR only if the passed in uidCheck is -1 or matches + // the internal mAllowUid. + // An external submit will always have a valid uidCheck parameter. + // An internal get request within mediametrics will have a uidCheck == -1 which + // we allow to proceed. status_t checkPermission(uid_t uidCheck) const { - return uidCheck != (uid_t)-1 && uidCheck != mUid ? PERMISSION_DENIED : NO_ERROR; + return uidCheck != (uid_t)-1 && uidCheck != mAllowUid ? PERMISSION_DENIED : NO_ERROR; } template @@ -199,8 +208,7 @@ private: } const std::string mKey; - const pid_t mPid __unused; - const uid_t mUid; + const uid_t mAllowUid; const int64_t mCreationTime __unused; int64_t mLastModificationTime; @@ -276,10 +284,13 @@ public: (void)gc(garbage); + // We set the allowUid for client access on key creation. + int32_t allowUid = -1; + (void)item->get(AMEDIAMETRICS_PROP_ALLOWUID, &allowUid); // no keylock needed here as we are sole owner // until placed on mHistory. keyHistory = std::make_shared( - key, item->getPid(), item->getUid(), time); + key, allowUid, time); mHistory[key] = keyHistory; } else { keyHistory = it->second; diff --git a/services/mediametrics/tests/mediametrics_tests.cpp b/services/mediametrics/tests/mediametrics_tests.cpp index 78eb71c90c..cf0dceb9f8 100644 --- a/services/mediametrics/tests/mediametrics_tests.cpp +++ b/services/mediametrics/tests/mediametrics_tests.cpp @@ -813,6 +813,42 @@ TEST(mediametrics_tests, audio_analytics_permission) { ASSERT_LT(9, audioAnalytics.dump(1000).second /* lines */); } +TEST(mediametrics_tests, audio_analytics_permission2) { + constexpr int32_t transactionUid = 1010; // arbitrary + auto item = std::make_shared("audio.1"); + (*item).set("one", (int32_t)1) + .set("two", (int32_t)2) + .set(AMEDIAMETRICS_PROP_ALLOWUID, transactionUid) + .setTimestamp(10); + + // item2 submitted untrusted + auto item2 = std::make_shared("audio.1"); + (*item2).set("three", (int32_t)3) + .setUid(transactionUid) + .setTimestamp(11); + + auto item3 = std::make_shared("audio.2"); + (*item3).set("four", (int32_t)4) + .setTimestamp(12); + + android::mediametrics::AudioAnalytics audioAnalytics; + + // untrusted entities cannot create a new key. + ASSERT_EQ(PERMISSION_DENIED, audioAnalytics.submit(item, false /* isTrusted */)); + ASSERT_EQ(PERMISSION_DENIED, audioAnalytics.submit(item2, false /* isTrusted */)); + + // TODO: Verify contents of AudioAnalytics. + // Currently there is no getter API in AudioAnalytics besides dump. + ASSERT_EQ(9, audioAnalytics.dump(1000).second /* lines */); + + ASSERT_EQ(NO_ERROR, audioAnalytics.submit(item, true /* isTrusted */)); + // untrusted entities can add to an existing key + ASSERT_EQ(NO_ERROR, audioAnalytics.submit(item2, false /* isTrusted */)); + + // Check that we have some info in the dump. + ASSERT_LT(9, audioAnalytics.dump(1000).second /* lines */); +} + TEST(mediametrics_tests, audio_analytics_dump) { auto item = std::make_shared("audio.1"); (*item).set("one", (int32_t)1) diff --git a/services/oboeservice/AAudioServiceStreamBase.cpp b/services/oboeservice/AAudioServiceStreamBase.cpp index 044c361d12..dba9fb9fee 100644 --- a/services/oboeservice/AAudioServiceStreamBase.cpp +++ b/services/oboeservice/AAudioServiceStreamBase.cpp @@ -105,6 +105,7 @@ void AAudioServiceStreamBase::logOpen(aaudio_handle_t streamHandle) { mediametrics::LogItem(mMetricsId) .setPid(getOwnerProcessId()) .setUid(getOwnerUserId()) + .set(AMEDIAMETRICS_PROP_ALLOWUID, (int32_t)getOwnerUserId()) .set(AMEDIAMETRICS_PROP_EVENT, AMEDIAMETRICS_PROP_EVENT_VALUE_OPEN) // the following are immutable .set(AMEDIAMETRICS_PROP_BUFFERCAPACITYFRAMES, (int32_t)getBufferCapacity())