Bug 1705278 - Remove DependentModules from UntrustedModulesProcessor.cpp. r=aklotz

Bug 1620118 added a new field `isDependent` in the third-party-module ping
which is calculated in `UntrustedModulesProcessor`.  However, bug 1684532
revealed it was not accurate because some third-party applications revert
the import table to the original state immediately after their module was
loaded.

Now that we have a logic to determine `isDependent` in `NtMapViewOfSection`
to automatically block a module injected through the import table, we can
pass that value to the ping and remove the original logic in `UntrustedModulesProcessor`.

Differential Revision: https://phabricator.services.mozilla.com/D112227

browser/app/winlauncher/freestanding/DllBlocklist.cpp

@@ -467,7 +467,7 @@ NTSTATUS NTAPI patched_NtMapViewOfSection(
   if (nt::RtlGetProcessHeap()) {
     ModuleLoadFrame::NotifySectionMap(
         nt::AllocatedUnicodeString(sectionFileName), *aBaseAddress, stubStatus,
-        loadStatus);
+        loadStatus, isDependent);
   }
 
   if (loadStatus == ModuleLoadInfo::Status::Loaded ||

browser/app/winlauncher/freestanding/ModuleLoadFrame.cpp

@@ -25,12 +25,14 @@ ModuleLoadFrame::ModuleLoadFrame(PCUNICODE_STRING aRequestedDllName)
 
 ModuleLoadFrame::ModuleLoadFrame(nt::AllocatedUnicodeString&& aSectionName,
                                  const void* aMapBaseAddr, NTSTATUS aNtStatus,
-                                 ModuleLoadInfo::Status aLoadStatus)
+                                 ModuleLoadInfo::Status aLoadStatus,
+                                 bool aIsDependent)
     : mPrev(sTopFrame.get()),
       mContext(nullptr),
       mLSPSubstitutionRequired(false),
       mLoadNtStatus(aNtStatus),
-      mLoadInfo(std::move(aSectionName), aMapBaseAddr, aLoadStatus) {
+      mLoadInfo(std::move(aSectionName), aMapBaseAddr, aLoadStatus,
+                aIsDependent) {
   sTopFrame.set(this);
 
   gLoaderPrivateAPI.NotifyBeginDllLoad(&mContext, mLoadInfo.mSectionName);
@@ -70,7 +72,8 @@ void ModuleLoadFrame::SetLSPSubstitutionRequired(PCUNICODE_STRING aLeafName) {
 /* static */
 void ModuleLoadFrame::NotifySectionMap(
     nt::AllocatedUnicodeString&& aSectionName, const void* aMapBaseAddr,
-    NTSTATUS aMapNtStatus, ModuleLoadInfo::Status aLoadStatus) {
+    NTSTATUS aMapNtStatus, ModuleLoadInfo::Status aLoadStatus,
+    bool aIsDependent) {
   ModuleLoadFrame* topFrame = sTopFrame.get();
   if (!topFrame) {
     // The only time that this data is useful is during initial mapping of
@@ -79,13 +82,13 @@ void ModuleLoadFrame::NotifySectionMap(
     // initial process startup.
     if (gLoaderPrivateAPI.IsDefaultObserver()) {
       OnBareSectionMap(std::move(aSectionName), aMapBaseAddr, aMapNtStatus,
-                       aLoadStatus);
+                       aLoadStatus, aIsDependent);
     }
     return;
   }
 
   topFrame->OnSectionMap(std::move(aSectionName), aMapBaseAddr, aMapNtStatus,
-                         aLoadStatus);
+                         aLoadStatus, aIsDependent);
 }
 
 /* static */
@@ -94,12 +97,13 @@ bool ModuleLoadFrame::ExistsTopFrame() { return !!sTopFrame.get(); }
 void ModuleLoadFrame::OnSectionMap(nt::AllocatedUnicodeString&& aSectionName,
                                    const void* aMapBaseAddr,
                                    NTSTATUS aMapNtStatus,
-                                   ModuleLoadInfo::Status aLoadStatus) {
+                                   ModuleLoadInfo::Status aLoadStatus,
+                                   bool aIsDependent) {
   if (mLoadInfo.mBaseAddr) {
     // If mBaseAddr is not null then |this| has already seen a module load. This
     // means that we are witnessing a bare section map.
     OnBareSectionMap(std::move(aSectionName), aMapBaseAddr, aMapNtStatus,
-                     aLoadStatus);
+                     aLoadStatus, aIsDependent);
     return;
   }
 
@@ -111,10 +115,11 @@ void ModuleLoadFrame::OnSectionMap(nt::AllocatedUnicodeString&& aSectionName,
 /* static */
 void ModuleLoadFrame::OnBareSectionMap(
     nt::AllocatedUnicodeString&& aSectionName, const void* aMapBaseAddr,
-    NTSTATUS aMapNtStatus, ModuleLoadInfo::Status aLoadStatus) {
+    NTSTATUS aMapNtStatus, ModuleLoadInfo::Status aLoadStatus,
+    bool aIsDependent) {
   // We call the special constructor variant that is used for bare mappings.
   ModuleLoadFrame frame(std::move(aSectionName), aMapBaseAddr, aMapNtStatus,
-                        aLoadStatus);
+                        aLoadStatus, aIsDependent);
 }
 
 NTSTATUS ModuleLoadFrame::SetLoadStatus(NTSTATUS aNtStatus,

browser/app/winlauncher/freestanding/ModuleLoadFrame.h

@@ -37,7 +37,8 @@ class MOZ_RAII ModuleLoadFrame final {
    */
   static void NotifySectionMap(nt::AllocatedUnicodeString&& aSectionName,
                                const void* aMapBaseAddr, NTSTATUS aMapNtStatus,
-                               ModuleLoadInfo::Status aLoadStatus);
+                               ModuleLoadInfo::Status aLoadStatus,
+                               bool aIsDependent);
   static bool ExistsTopFrame();
 
   /**
@@ -57,12 +58,12 @@ class MOZ_RAII ModuleLoadFrame final {
    */
   ModuleLoadFrame(nt::AllocatedUnicodeString&& aSectionName,
                   const void* aMapBaseAddr, NTSTATUS aNtStatus,
-                  ModuleLoadInfo::Status aLoadStatus);
+                  ModuleLoadInfo::Status aLoadStatus, bool aIsDependent);
 
   void SetLSPSubstitutionRequired(PCUNICODE_STRING aLeafName);
   void OnSectionMap(nt::AllocatedUnicodeString&& aSectionName,
                     const void* aMapBaseAddr, NTSTATUS aMapNtStatus,
-                    ModuleLoadInfo::Status aLoadStatus);
+                    ModuleLoadInfo::Status aLoadStatus, bool aIsDependent);
 
   /**
    * A "bare" section mapping is one that was mapped without the code passing
@@ -71,7 +72,8 @@ class MOZ_RAII ModuleLoadFrame final {
    */
   static void OnBareSectionMap(nt::AllocatedUnicodeString&& aSectionName,
                                const void* aMapBaseAddr, NTSTATUS aMapNtStatus,
-                               ModuleLoadInfo::Status aLoadStatus);
+                               ModuleLoadInfo::Status aLoadStatus,
+                               bool aIsDependent);
 
  private:
   // Link to the previous frame

mozglue/dllservices/ModuleLoadInfo.h

@@ -34,7 +34,8 @@ struct ModuleLoadInfo final {
         mThreadId(nt::RtlGetCurrentThreadId()),
         mRequestedDllName(aRequestedDllName),
         mBaseAddr(nullptr),
-        mStatus(Status::Loaded) {
+        mStatus(Status::Loaded),
+        mIsDependent(false) {
 #  if defined(IMPL_MFBT)
     ::QueryPerformanceCounter(&mBeginTimestamp);
 #  else
@@ -49,12 +50,13 @@ struct ModuleLoadInfo final {
    * of another library.
    */
   ModuleLoadInfo(nt::AllocatedUnicodeString&& aSectionName,
-                 const void* aBaseAddr, Status aLoadStatus)
+                 const void* aBaseAddr, Status aLoadStatus, bool aIsDependent)
       : mLoadTimeInfo(),
         mThreadId(nt::RtlGetCurrentThreadId()),
         mSectionName(std::move(aSectionName)),
         mBaseAddr(aBaseAddr),
-        mStatus(aLoadStatus) {
+        mStatus(aLoadStatus),
+        mIsDependent(aIsDependent) {
 #  if defined(IMPL_MFBT)
     ::QueryPerformanceCounter(&mBeginTimestamp);
 #  else
@@ -164,6 +166,8 @@ struct ModuleLoadInfo final {
   Vector<PVOID, 0, nt::RtlAllocPolicy> mBacktrace;
   // The status of DLL load
   Status mStatus;
+  // Whether the module is one of the executables's dependent modules or not
+  bool mIsDependent;
 };
 
 using ModuleLoadInfoVec = Vector<ModuleLoadInfo, 0, nt::RtlAllocPolicy>;

mozglue/misc/NativeNt.h

@@ -26,9 +26,7 @@
 #include "mozilla/interceptor/TargetFunction.h"
 
 #if defined(MOZILLA_INTERNAL_API)
-#  include "nsHashKeys.h"
 #  include "nsString.h"
-#  include "nsTHashtable.h"
 #endif  // defined(MOZILLA_INTERNAL_API)
 
 // The declarations within this #if block are intended to be used for initial
@@ -743,17 +741,6 @@ class MOZ_RAII PEHeaders final {
     }
   }
 
-#if defined(MOZILLA_INTERNAL_API)
-  nsTHashtable<nsStringCaseInsensitiveHashKey> GenerateDependentModuleSet()
-      const {
-    nsTHashtable<nsStringCaseInsensitiveHashKey> dependentModuleSet;
-    EnumImportChunks([&dependentModuleSet](const char* aModule) {
-      dependentModuleSet.PutEntry(GetLeafName(NS_ConvertASCIItoUTF16(aModule)));
-    });
-    return dependentModuleSet;
-  }
-#endif  // defined(MOZILLA_INTERNAL_API)
-
   /**
    * If |aBoundaries| is given, this method checks whether each IAT entry is
    * within the given range, and if any entry is out of the range, we return

mozglue/tests/gtest/TestNativeNtGTest.cpp

@@ -9,12 +9,19 @@
 #include "gtest/gtest.h"
 
 #include "mozilla/NativeNt.h"
+#include "nsHashKeys.h"
+#include "nsTHashSet.h"
 
 TEST(TestNativeNtGTest, GenerateDependentModuleSet)
 {
   mozilla::nt::PEHeaders executable(::GetModuleHandleW(nullptr));
-  auto dependentModules = executable.GenerateDependentModuleSet();
+  nsTHashSet<nsStringCaseInsensitiveHashKey> dependentModules;
-  EXPECT_NE(dependentModules.GetEntry(u"mozglue.dll"_ns), nullptr);
+  executable.EnumImportChunks([&](const char* aModule) {
-  EXPECT_NE(dependentModules.GetEntry(u"MOZGLUE.dll"_ns), nullptr);
+    dependentModules.Insert(
-  EXPECT_EQ(dependentModules.GetEntry(u"xxx.dll"_ns), nullptr);
+        mozilla::nt::GetLeafName(NS_ConvertASCIItoUTF16(aModule)));
+  });
+
+  EXPECT_TRUE(dependentModules.Contains(u"mozglue.dll"_ns));
+  EXPECT_TRUE(dependentModules.Contains(u"MOZGLUE.dll"_ns));
+  EXPECT_FALSE(dependentModules.Contains(u"xxx.dll"_ns));
 }

toolkit/xre/UntrustedModulesData.cpp

@@ -205,7 +205,7 @@ ProcessedModuleLoadEvent::ProcessedModuleLoadEvent()
 
 ProcessedModuleLoadEvent::ProcessedModuleLoadEvent(
     glue::EnhancedModuleLoadInfo&& aModLoadInfo,
-    RefPtr<ModuleRecord>&& aModuleRecord, bool aIsDependent)
+    RefPtr<ModuleRecord>&& aModuleRecord)
     : mProcessUptimeMS(QPCTimeStampToProcessUptimeMilliseconds(
           aModLoadInfo.mNtLoadInfo.mBeginTimestamp)),
       mLoadDurationMS(QPCLoadDurationToMilliseconds(aModLoadInfo.mNtLoadInfo)),
@@ -214,7 +214,7 @@ ProcessedModuleLoadEvent::ProcessedModuleLoadEvent(
       mBaseAddress(
           reinterpret_cast<uintptr_t>(aModLoadInfo.mNtLoadInfo.mBaseAddr)),
       mModule(std::move(aModuleRecord)),
-      mIsDependent(aIsDependent),
+      mIsDependent(aModLoadInfo.mNtLoadInfo.mIsDependent),
       mLoadStatus(static_cast<uint32_t>(aModLoadInfo.mNtLoadInfo.mStatus)) {
   if (!mModule || !(*mModule)) {
     return;

toolkit/xre/UntrustedModulesData.h

@@ -127,8 +127,7 @@ class ProcessedModuleLoadEvent final {
  public:
   ProcessedModuleLoadEvent();
   ProcessedModuleLoadEvent(glue::EnhancedModuleLoadInfo&& aModLoadInfo,
-                           RefPtr<ModuleRecord>&& aModuleRecord,
+                           RefPtr<ModuleRecord>&& aModuleRecord);
-                           bool aIsDependent);
 
   explicit operator bool() const { return mModule && *mModule; }
   bool IsXULLoad() const;

toolkit/xre/UntrustedModulesProcessor.cpp

@@ -87,34 +87,6 @@ class MOZ_RAII BackgroundPriorityRegion final {
   const BOOL mIsBackground;
 };
 
-// This class wraps a set of the executables's dependent modules
-// that is delay-initialized the first time Lookup() is called.
-class DependentModules final {
-  Maybe<nsTHashtable<nsStringCaseInsensitiveHashKey>> mDependentModules;
-
- public:
-  bool Lookup(const nsString& aModulePath) {
-    if (aModulePath.IsEmpty()) {
-      return false;
-    }
-
-    if (mDependentModules.isNothing()) {
-      nt::PEHeaders executable(::GetModuleHandleW(nullptr));
-
-      // We generate a hash table only when the executable's import table is
-      // tampered.  If the import table is intact, all dependent modules are
-      // legit and we're not interested in any of them.  In such a case, we
-      // set an empty table so that this function returns false.
-      mDependentModules =
-          Some(executable.IsImportDirectoryTampered()
-                   ? executable.GenerateDependentModuleSet()
-                   : nsTHashtable<nsStringCaseInsensitiveHashKey>());
-    }
-
-    return !!mDependentModules.ref().GetEntry(nt::GetLeafName(aModulePath));
-  }
-};
-
 /* static */
 bool UntrustedModulesProcessor::IsSupportedProcessType() {
   switch (XRE_GetProcessType()) {
@@ -150,8 +122,7 @@ UntrustedModulesProcessor::UntrustedModulesProcessor()
                                  LazyIdleThread::ManualShutdown)),
       mUnprocessedMutex(
           "mozilla::UntrustedModulesProcessor::mUnprocessedMutex"),
-      mAllowProcessing(true),
+      mAllowProcessing(true) {
-      mIsFirstBatchProcessed(false) {
   AddObservers();
 }
 
@@ -595,11 +566,8 @@ void UntrustedModulesProcessor::ProcessModuleLoadQueue() {
     return;
   }
 
-  auto cleanup = MakeScopeExit([&]() { mIsFirstBatchProcessed = true; });
-
   Telemetry::BatchProcessedStackGenerator stackProcessor;
   ModulesMap modules;
-  DependentModules dependentModules;
 
   Maybe<double> maybeXulLoadDuration;
   Vector<Telemetry::ProcessedStack> processedStacks;
@@ -624,18 +592,9 @@ void UntrustedModulesProcessor::ProcessModuleLoadQueue() {
       return;
     }
 
-    bool isDependent = mIsFirstBatchProcessed
-                           ? false
-                           : dependentModules.Lookup(module->mSanitizedDllName);
-
-    if (!mAllowProcessing) {
-      return;
-    }
-
     glue::EnhancedModuleLoadInfo::BacktraceType backtrace =
         std::move(entry.mNtLoadInfo.mBacktrace);
-    ProcessedModuleLoadEvent event(std::move(entry), std::move(module),
+    ProcessedModuleLoadEvent event(std::move(entry), std::move(module));
-                                   isDependent);
 
     if (!event) {
       // We don't have a sanitized DLL path, so we cannot include this event
@@ -857,10 +816,7 @@ void UntrustedModulesProcessor::CompleteProcessing(
     return;
   }
 
-  auto cleanup = MakeScopeExit([&]() { mIsFirstBatchProcessed = true; });
-
   Telemetry::BatchProcessedStackGenerator stackProcessor;
-  DependentModules dependentModules;
 
   Maybe<double> maybeXulLoadDuration;
   Vector<Telemetry::ProcessedStack> processedStacks;
@@ -883,19 +839,9 @@ void UntrustedModulesProcessor::CompleteProcessing(
         return;
       }
 
-      bool isDependent =
-          mIsFirstBatchProcessed
-              ? false
-              : dependentModules.Lookup(module->mSanitizedDllName);
-
-      if (!mAllowProcessing) {
-        return;
-      }
-
       glue::EnhancedModuleLoadInfo::BacktraceType backtrace =
           std::move(item.mNtLoadInfo.mBacktrace);
-      ProcessedModuleLoadEvent event(std::move(item), std::move(module),
+      ProcessedModuleLoadEvent event(std::move(item), std::move(module));
-                                     isDependent);
 
       if (!mAllowProcessing) {
         return;

toolkit/xre/UntrustedModulesProcessor.h

@@ -141,15 +141,6 @@ class UntrustedModulesProcessor final : public nsIObserver {
 
   // This member may be touched by any thread
   Atomic<bool> mAllowProcessing;
-
-  // This member must only be touched on mThread, making sure a hash table of
-  // dependent modules is initialized once in a process when the first batch
-  // of queued events is processed.  We don't need the hash table to process
-  // subsequent queues because we're interested in modules imported via the
-  // executable's Import Table which are all expected to be in the first queue.
-  // A boolean flag is enough to control initialization because tasks of
-  // processing queues are serialized.
-  bool mIsFirstBatchProcessed;
 };
 
 }  // namespace mozilla