From 0c4a21e4eaef1b60a1860d889bc37c81437cb055 Mon Sep 17 00:00:00 2001
From: Tej Singh <singhtejinder@google.com>
Date: Fri, 20 Sep 2019 19:28:53 -0700
Subject: [PATCH] Security Fix: Race Condition + NPE

ShellSubscriber is lazily initialized, and multiple threads can attempt
to write the same pointer since it is not initialized in threadsafe
code. Additionally, there is an NPE that crashes statsd when a null
ResultReceiver is passed in, which allows an attacker to repeatedly
crash statsd until the race condition occurs. More details, including a
proof of concept attack, are in the bug.

Bug: 141243101
Test: repro steps in bug no longer crash statsd
Test: with only the lock on iniitiallizing mShellSubscriber, statsd
still crashed but after ~7 minutes, no race condition occurred.

Change-Id: Ib56f888620497fb41d1627c07867693eb251738e
---
 cmds/statsd/src/StatsService.cpp | 15 ++++++++++++---
 cmds/statsd/src/StatsService.h   |  4 ++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/cmds/statsd/src/StatsService.cpp b/cmds/statsd/src/StatsService.cpp
index 1c7180ffbde1f..b665a8b99fbd3 100644
--- a/cmds/statsd/src/StatsService.cpp
+++ b/cmds/statsd/src/StatsService.cpp
@@ -266,7 +266,9 @@ status_t StatsService::onTransact(uint32_t code, const Parcel& data, Parcel* rep
                     IResultReceiver::asInterface(data.readStrongBinder());
 
             err = command(in, out, err, args, resultReceiver);
-            resultReceiver->send(err);
+            if (resultReceiver != nullptr) {
+                resultReceiver->send(err);
+            }
             return NO_ERROR;
         }
         default: { return BnStatsManager::onTransact(code, data, reply, flags); }
@@ -411,13 +413,20 @@ status_t StatsService::command(int in, int out, int err, Vector<String8>& args,
             return cmd_trigger_active_config_broadcast(out, args);
         }
         if (!args[0].compare(String8("data-subscribe"))) {
-            if (mShellSubscriber == nullptr) {
-                mShellSubscriber = new ShellSubscriber(mUidMap, mPullerManager);
+            {
+                std::lock_guard<std::mutex> lock(mShellSubscriberMutex);
+                if (mShellSubscriber == nullptr) {
+                    mShellSubscriber = new ShellSubscriber(mUidMap, mPullerManager);
+                }
             }
             int timeoutSec = -1;
             if (argCount >= 2) {
                 timeoutSec = atoi(args[1].c_str());
             }
+            if (resultReceiver == nullptr) {
+                ALOGI("Null resultReceiver given, no subscription will be started");
+                return UNEXPECTED_NULL;
+            }
             mShellSubscriber->startNewSubscription(in, out, resultReceiver, timeoutSec);
             return NO_ERROR;
         }
diff --git a/cmds/statsd/src/StatsService.h b/cmds/statsd/src/StatsService.h
index 53b6ce989195f..9490948719363 100644
--- a/cmds/statsd/src/StatsService.h
+++ b/cmds/statsd/src/StatsService.h
@@ -432,6 +432,10 @@ private:
 
     sp<ShellSubscriber> mShellSubscriber;
 
+    /**
+     * Mutex for setting the shell subscriber
+     */
+    mutable mutex mShellSubscriberMutex;
     std::shared_ptr<LogEventQueue> mEventQueue;
 
     FRIEND_TEST(StatsLogProcessorTest, TestActivationsPersistAcrossSystemServerRestart);