[Backport] CVE-2019-5789

Web MIDI: Make TaskService and MidiManagerWin integer-overflow-proof

TaskService and MidiManagerWin uses int to identify the instance,
but this change makes it int64_t and improve them to fail gracefully
without shutting-down the browser due to CHECK failures.

For practical use, the original int is enough and it's impossible
to overflow the instance ID unless attackers success to run their code
for several months on an occupied active processor.

Bug: 921581
Reviewed-on: https://chromium-review.googlesource.com/c/1449483
Change-Id: Ia721bf8ba705c8a132d354aed239e990600d6532
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

4 files changed, 76 insertions, 41 deletions

diff --git a/chromium/media/midi/midi_manager_win.cc b/chromium/media/midi/midi_manager_win.cc
index 7d507bdabda..cb0bdc0ee32 100644
--- a/chromium/media/midi/midi_manager_win.cc
+++ b/chromium/media/midi/midi_manager_win.cc
@@ -102,8 +102,8 @@ constexpr size_t kSysExSizeLimit = 256 * 1024;
 constexpr size_t kBufferLength = 32 * 1024;
 
 // Global variables to identify MidiManager instance.
-constexpr int kInvalidInstanceId = -1;
-int g_active_instance_id = kInvalidInstanceId;
+constexpr int64_t kInvalidInstanceId = -1;
+int64_t g_active_instance_id = kInvalidInstanceId;
 MidiManagerWin* g_manager_instance = nullptr;
 
 // Obtains base::Lock instance pointer to lock instance_id.
@@ -113,8 +113,15 @@ base::Lock* GetInstanceIdLock() {
 }
 
 // Issues unique MidiManager instance ID.
-int IssueNextInstanceId() {
-  static int id = kInvalidInstanceId;
+int64_t IssueNextInstanceId(base::Optional<int64_t> override_id) {
+  static int64_t id = kInvalidInstanceId;
+  if (override_id) {
+    int64_t result = ++id;
+    id = *override_id;
+    return result;
+  }
+  if (id == std::numeric_limits<int64_t>::max())
+    return kInvalidInstanceId;
   return ++id;
 }
 
@@ -685,9 +692,14 @@ MidiManagerWin::PortManager::HandleMidiOutCallback(HMIDIOUT hmo,
   }
 }
 
+// static
+void MidiManagerWin::OverflowInstanceIdForTesting() {
+  IssueNextInstanceId(std::numeric_limits<int64_t>::max());
+}
+
 MidiManagerWin::MidiManagerWin(MidiService* service)
     : MidiManager(service),
-      instance_id_(IssueNextInstanceId()),
+      instance_id_(IssueNextInstanceId(base::nullopt)),
       port_manager_(std::make_unique<PortManager>()) {
   base::AutoLock lock(*GetInstanceIdLock());
   CHECK_EQ(kInvalidInstanceId, g_active_instance_id);
@@ -705,6 +717,9 @@ MidiManagerWin::~MidiManagerWin() {
 void MidiManagerWin::StartInitialization() {
   {
     base::AutoLock lock(*GetInstanceIdLock());
+    if (instance_id_ == kInvalidInstanceId)
+      return CompleteInitialization(mojom::Result::INITIALIZATION_ERROR);
+
     CHECK_EQ(kInvalidInstanceId, g_active_instance_id);
     g_active_instance_id = instance_id_;
     CHECK_EQ(nullptr, g_manager_instance);
@@ -720,7 +735,11 @@ void MidiManagerWin::StartInitialization() {
 }
 
 void MidiManagerWin::Finalize() {
-  // Unregisters on the I/O thread. OnDevicesChanged() won't be called any more.
+
+  if (instance_id_ == kInvalidInstanceId)
+     return;
+
+  // Unregisters on the I/O thread. OnDevicesChanged() won't be called any more
   CHECK(thread_runner_->BelongsToCurrentThread());
   base::SystemMonitor::Get()->RemoveDevicesChangedObserver(this);
   {
diff --git a/chromium/media/midi/midi_manager_win.h b/chromium/media/midi/midi_manager_win.h
index f3f6093f603..3d23a59fa50 100644
--- a/chromium/media/midi/midi_manager_win.h
+++ b/chromium/media/midi/midi_manager_win.h
@@ -28,6 +28,8 @@ class MidiManagerWin final
  public:
   class PortManager;
 
+  MIDI_EXPORT static void OverflowInstanceIdForTesting();
+
   explicit MidiManagerWin(MidiService* service);
   ~MidiManagerWin() override;
 
@@ -84,7 +86,7 @@ class MidiManagerWin final
                         const std::vector<uint8_t>& data);
 
   // Holds an unique instance ID.
-  const int instance_id_;
+  const int64_t instance_id_;
 
   // Keeps a TaskRunner for the I/O thread.
   scoped_refptr<base::SingleThreadTaskRunner> thread_runner_;
diff --git a/chromium/media/midi/task_service.cc b/chromium/media/midi/task_service.cc
index 3bb3baf01b5..563d63fbf91 100644
--- a/chromium/media/midi/task_service.cc
+++ b/chromium/media/midi/task_service.cc
@@ -12,19 +12,10 @@
 
 namespace midi {
 
-namespace {
-
-constexpr TaskService::InstanceId kInvalidInstanceId = -1;
-
-}  // namespace
-
 constexpr TaskService::RunnerId TaskService::kDefaultRunnerId;
+constexpr TaskService::InstanceId TaskService::kInvalidInstanceId;
 
-TaskService::TaskService()
-    : no_tasks_in_flight_cv_(&tasks_in_flight_lock_),
-      tasks_in_flight_(0),
-      next_instance_id_(0),
-      bound_instance_id_(kInvalidInstanceId) {
+TaskService::TaskService() : no_tasks_in_flight_cv_(&tasks_in_flight_lock_) {
   DETACH_FROM_SEQUENCE(instance_binding_sequence_checker_);
 }
 
@@ -45,10 +36,17 @@ bool TaskService::BindInstance() {
   base::AutoLock lock(lock_);
   if (bound_instance_id_ != kInvalidInstanceId)
     return false;
-  bound_instance_id_ = next_instance_id_++;
+
+  // If the InstanceId reaches to the limit, just fail rather than doing
+  // something nicer for such impractical case.
+  if (std::numeric_limits<InstanceId>::max() == next_instance_id_)
+    return false;
+
+  bound_instance_id_ = ++next_instance_id_;
 
   DCHECK(!default_task_runner_);
   default_task_runner_ = base::ThreadTaskRunnerHandle::Get();
+
   return true;
 }
 
@@ -58,16 +56,17 @@ bool TaskService::UnbindInstance() {
     base::AutoLock lock(lock_);
     if (bound_instance_id_ == kInvalidInstanceId)
       return false;
+
+    DCHECK_EQ(next_instance_id_, bound_instance_id_);
     bound_instance_id_ = kInvalidInstanceId;
 
     DCHECK(default_task_runner_);
     default_task_runner_ = nullptr;
   }
-
   // From now on RunTask will never run any task bound to the instance id.
   // But invoked tasks might be still running here. To ensure no task runs on
   // quitting this method, wait for all tasks to complete.
-  base::AutoLock tasks_in_flight_auto_lock(tasks_in_flight_lock_);
+  base::AutoLock tasks_in_flight_lock(tasks_in_flight_lock_);
   // TODO(https://crbug.com/796830): Remove sync operations on the I/O thread.
   base::ScopedAllowBaseSyncPrimitivesOutsideBlockingScope allow_wait;
   while (tasks_in_flight_ > 0)
@@ -126,6 +125,10 @@ void TaskService::PostBoundDelayedTask(RunnerId runner_id,
       delay);
 }
 
+void TaskService::OverflowInstanceIdForTesting() {
+  next_instance_id_ = std::numeric_limits<InstanceId>::max();
+}
+
 scoped_refptr<base::SingleThreadTaskRunner> TaskService::GetTaskRunner(
     RunnerId runner_id) {
   base::AutoLock lock(lock_);
@@ -154,7 +157,7 @@ void TaskService::RunTask(InstanceId instance_id,
                           RunnerId runner_id,
                           base::OnceClosure task) {
   {
-    base::AutoLock tasks_in_flight_auto_lock(tasks_in_flight_lock_);
+    base::AutoLock tasks_in_flight_lock(tasks_in_flight_lock_);
     ++tasks_in_flight_;
   }
 
@@ -162,7 +165,7 @@ void TaskService::RunTask(InstanceId instance_id,
     std::move(task).Run();
 
   {
-    base::AutoLock tasks_in_flight_auto_lock(tasks_in_flight_lock_);
+    base::AutoLock tasks_in_flight_lock(tasks_in_flight_lock_);
     --tasks_in_flight_;
     DCHECK_GE(tasks_in_flight_, 0);
     if (tasks_in_flight_ == 0)
diff --git a/chromium/media/midi/task_service.h b/chromium/media/midi/task_service.h
index 0845b966bdc..4ee02fe3435 100644
--- a/chromium/media/midi/task_service.h
+++ b/chromium/media/midi/task_service.h
@@ -5,13 +5,18 @@
 #ifndef MEDIA_MIDI_TASK_SERVICE_H_
 #define MEDIA_MIDI_TASK_SERVICE_H_
 
+#include <memory>
+#include <vector>
+
 #include "base/callback_forward.h"
+#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/sequence_checker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "media/midi/midi_export.h"
@@ -23,7 +28,7 @@ namespace midi {
 class MIDI_EXPORT TaskService final {
  public:
   using RunnerId = size_t;
-  using InstanceId = int;
+  using InstanceId = int64_t;
 
   static constexpr RunnerId kDefaultRunnerId = 0;
 
@@ -36,9 +41,10 @@ class MIDI_EXPORT TaskService final {
   // invoked any more.
   // Returns true if call is bound or unbound correctly. Otherwise returns
   // false, that happens when the BindInstance() is called twice without
-  // unbinding the previous instance.
-  bool BindInstance();
-  bool UnbindInstance();
+  // unbinding the previous instance, or the UnbindInstance() is called without
+  // any successful BindInstance() call.
+  bool BindInstance() WARN_UNUSED_RESULT;
+  bool UnbindInstance() WARN_UNUSED_RESULT;
 
   // Checks if the current thread belongs to the specified runner.
   bool IsOnTaskRunner(RunnerId runner_id);
@@ -58,7 +64,11 @@ class MIDI_EXPORT TaskService final {
                             base::OnceClosure task,
                             base::TimeDelta delay);
 
+  void OverflowInstanceIdForTesting();
+
  private:
+  static constexpr TaskService::InstanceId kInvalidInstanceId = -1;
+
   // Returns a SingleThreadTaskRunner reference. Each TaskRunner will be
   // constructed on demand.
   scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner(RunnerId runner_id);
@@ -71,31 +81,32 @@ class MIDI_EXPORT TaskService final {
   // Returns true if |instance_id| is equal to |bound_instance_id_|.
   bool IsInstanceIdStillBound(InstanceId instance_id);
 
+  // Holds InstanceId for the next bound instance, accessed on the BindInstance
+  // call thread without any protection.
+  InstanceId next_instance_id_ = kInvalidInstanceId;
+
   // Keeps a TaskRunner for the thread that calls BindInstance() as a default
   // task runner to run posted tasks.
-  scoped_refptr<base::SingleThreadTaskRunner> default_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> default_task_runner_
+      GUARDED_BY(lock_);
 
   // Holds threads to host SingleThreadTaskRunners.
-  std::vector<std::unique_ptr<base::Thread>> threads_;
+  std::vector<std::unique_ptr<base::Thread>> threads_ GUARDED_BY(lock_);
 
-  // Protects |tasks_in_flight_|.
-  base::Lock tasks_in_flight_lock_;
+  // Holds InstanceId for the current bound instance.
+  InstanceId bound_instance_id_ GUARDED_BY(lock_) = kInvalidInstanceId;
+
+  base::Lock lock_;
 
   // Signalled when the number of tasks in flight is 0 and ensures that
   // UnbindInstance() does not return until all tasks have completed.
-  base::ConditionVariable no_tasks_in_flight_cv_;
+  base::ConditionVariable no_tasks_in_flight_cv_
+      GUARDED_BY(tasks_in_flight_lock_);
 
   // Number of tasks in flight.
-  int tasks_in_flight_;
-
-  // Holds InstanceId for the next bound instance.
-  InstanceId next_instance_id_;
-
-  // Holds InstanceId for the current bound instance.
-  InstanceId bound_instance_id_;
+  int tasks_in_flight_ GUARDED_BY(tasks_in_flight_lock_) = 0;
 
-  // Protects all members other than |tasks_in_flight_|.
-  base::Lock lock_;
+  base::Lock tasks_in_flight_lock_;
 
   // Verifies all UnbindInstance() calls occur on the same sequence as
   // BindInstance().