Bug 2057112: Part 4 - Add DLL-reporting failures to the third-party-modules ping r=win-reviewers,TravisLong,gstoll

Now that the parent identifies a child process's modules from the section
handle the child sends, and silently skips any handle it cannot identify,
we will need telemetry for how often that skipping happens. This patch
adds counters for that.

unverifiableLoads: the number of module loads in child processes where the DLL
blocklist hook reached the load but failed to duplicate the section handle.

rejectedSections: the number of module section handles a child process sent
to the parent that it refused as invalid module section handles.

The counters are per-process and are only ever incremented for child
processes, so the browser process reports zero for both. The counters are
reported by the process using the module, so their accuracy depends on
trusting the process itself. This is how the UntrustedModulesProcessor
has always worked, and is by design (for performance reasons).

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

toolkit/components/telemetry/docs/obsolete/collection/third-party-modules-ping.md

@@ -43,6 +43,11 @@ were loaded into Firefox processes.
         "sanitizationFailures": <int>,
         // Number of dropped events due to failures computing trust levels.
         "trustTestFailures": <int>,
+        // Number of child process module loads where the DLL blocklist hook reached
+        // the load but could not duplicate the module's section.
+        "unverifiableLoads": <int>,
+        // Number of sections a child process sent that the parent refused as invalid.
+        "rejectedSections": <int>,
         // Array of module load events for this process. The entries of this array are ordered to be in sync with the combinedStacks.stacks array (see below)
         "events": [
           {

toolkit/components/telemetry/metrics.yaml

@@ -1325,6 +1325,8 @@ third.party.modules:
         xulLoadDurationMS: number of milliseconds it took to load xul.dll,
         sanitizationFailures: number of dropped events due to failures in path sanitization,
         trustTestFailures: number of dropped events due to failures computing trust levels,
+        unverifiableLoads: number of module loads in child processes where the DLL blocklist hook reached the load but failed to duplicate the section handle,
+        rejectedSections: number of module section handles a child process sent that the parent refused as invalid module section handles,
         events: [{ // Array of module load events for this process. The entries of this array are ordered to be in sync with the combinedStacks.stacks array (see below).
           processUptimeMS: number of milliseconds between process creation and when this event was generated,
           loadDurationMS: number of milliseconds of time spent loading this module,
@@ -1354,6 +1356,7 @@ third.party.modules:
       ```
     bugs:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1963853
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=2057112
     data_reviews:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1963853
     data_sensitivity:
@@ -1380,6 +1383,10 @@ third.party.modules:
             type: number
           trustTestFailures:
             type: number
+          unverifiableLoads:
+            type: number
+          rejectedSections:
+            type: number
           events:
             type: array
             items:

toolkit/components/telemetry/other/UntrustedModules.cpp

@@ -287,6 +287,8 @@ nsresult MultiGetUntrustedModulesData::SubmitToGlean() {
         .processType = Some(processType),
         .sanitizationFailures = Some(data.mSanitizationFailures),
         .trustTestFailures = Some(data.mTrustTestFailures),
+        .unverifiableLoads = Some(data.mUnverifiableLoads),
+        .rejectedSections = Some(data.mRejectedSections),
     };
 
     nsCString strPid(processType);

toolkit/components/telemetry/other/UntrustedModulesDataSerializer.cpp

@@ -374,6 +374,20 @@ nsresult UntrustedModulesDataSerializer::GetPerProcObject(
     return NS_ERROR_FAILURE;
   }
 
+  JS::Rooted<JS::Value> jsUnverifiableLoads(mCx);
+  jsUnverifiableLoads.setNumber(aData.mUnverifiableLoads);
+  if (!JS_DefineProperty(mCx, aObj, "unverifiableLoads", jsUnverifiableLoads,
+                         JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+
+  JS::Rooted<JS::Value> jsRejectedSections(mCx);
+  jsRejectedSections.setNumber(aData.mRejectedSections);
+  if (!JS_DefineProperty(mCx, aObj, "rejectedSections", jsRejectedSections,
+                         JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+
   JS::Rooted<JSObject*> eventsArray(mCx);
   if (!ContainerToJSArray(mCx, &eventsArray, aData.mEvents, &SerializeEvent,
                           mIndexMap)) {

toolkit/components/telemetry/tests/unit/test_ThirdPartyModulesPing.js

@@ -126,6 +126,10 @@ add_task(async function test_send_ping() {
     "'sanitizationFailures' is 0"
   );
   Assert.equal(curProcInfo.trustTestFailures, 0, "'trustTestFailures' is 0");
+  // These two are only ever incremented for child processes but this test
+  // inspects the browser process's own data, so they are always zero.
+  Assert.equal(curProcInfo.unverifiableLoads, 0, "'unverifiableLoads' is 0");
+  Assert.equal(curProcInfo.rejectedSections, 0, "'rejectedSections' is 0");
 
   Assert.equal(
     curProcInfo.combinedStacks.stacks.length,

toolkit/xre/dllservices/UntrustedModulesData.h

@@ -191,7 +191,9 @@ class UntrustedModulesData final {
         mPid(::GetCurrentProcessId()),
         mNumEvents(0),
         mSanitizationFailures(0),
-        mTrustTestFailures(0) {
+        mTrustTestFailures(0),
+        mUnverifiableLoads(0),
+        mRejectedSections(0) {
     MOZ_ASSERT(kMaxEvents == mStacks.GetMaxStacksCount());
   }
 
@@ -203,6 +205,7 @@ class UntrustedModulesData final {
 
   explicit operator bool() const {
     return !mEvents.isEmpty() || mSanitizationFailures || mTrustTestFailures ||
+           mUnverifiableLoads || mRejectedSections ||
            mXULLoadDurationMS.isSome();
   }
 
@@ -226,11 +229,16 @@ class UntrustedModulesData final {
   Maybe<double> mXULLoadDurationMS;
   uint32_t mSanitizationFailures;
   uint32_t mTrustTestFailures;
+  // Counts cases where the child process couldn't duplicate the section
+  // handle.  See ModuleLoadInfo::mSectionHandleUnavailable.
+  uint32_t mUnverifiableLoads;
+  // Number of sections a child sent that the parent refused as invalid.
+  uint32_t mRejectedSections;
 };
 
 class ModulesMapResult final {
  public:
-  ModulesMapResult() : mTrustTestFailures(0) {}
+  ModulesMapResult() : mTrustTestFailures(0), mRejectedSections(0) {}
 
   ModulesMapResult(const ModulesMapResult& aOther) = delete;
   ModulesMapResult(ModulesMapResult&& aOther) = default;
@@ -239,6 +247,7 @@ class ModulesMapResult final {
 
   ModulesMap mModules;
   uint32_t mTrustTestFailures;
+  uint32_t mRejectedSections;
 };
 
 }  // namespace mozilla
@@ -394,6 +403,8 @@ struct ParamTraits<mozilla::UntrustedModulesData> {
     WriteParam(aWriter, aParam.mXULLoadDurationMS);
     aWriter->WriteUInt32(aParam.mSanitizationFailures);
     aWriter->WriteUInt32(aParam.mTrustTestFailures);
+    aWriter->WriteUInt32(aParam.mUnverifiableLoads);
+    aWriter->WriteUInt32(aParam.mRejectedSections);
   }
 
   static bool Read(MessageReader* aReader, paramType* aResult) {
@@ -447,6 +458,14 @@ struct ParamTraits<mozilla::UntrustedModulesData> {
       return false;
     }
 
+    if (!aReader->ReadUInt32(&aResult->mUnverifiableLoads)) {
+      return false;
+    }
+
+    if (!aReader->ReadUInt32(&aResult->mRejectedSections)) {
+      return false;
+    }
+
     return true;
   }
 
@@ -535,6 +554,7 @@ struct ParamTraits<mozilla::ModulesMapResult> {
   static void Write(MessageWriter* aWriter, const paramType& aParam) {
     WriteParam(aWriter, aParam.mModules);
     aWriter->WriteUInt32(aParam.mTrustTestFailures);
+    aWriter->WriteUInt32(aParam.mRejectedSections);
   }
 
   static bool Read(MessageReader* aReader, paramType* aResult) {
@@ -546,6 +566,10 @@ struct ParamTraits<mozilla::ModulesMapResult> {
       return false;
     }
 
+    if (!aReader->ReadUInt32(&aResult->mRejectedSections)) {
+      return false;
+    }
+
     return true;
   }
 };

toolkit/xre/dllservices/UntrustedModulesProcessor.cpp

@@ -718,6 +718,10 @@ RefPtr<ModuleRecord> UntrustedModulesProcessor::GetModuleRecord(
     const glue::EnhancedModuleLoadInfo& aModuleLoadInfo) {
   MOZ_ASSERT(!XRE_IsParentProcess());
 
+  // aModules is keyed by the path the parent derived with GetMappedFileNameW,
+  // while mSectionName comes from NtQueryVirtualMemory(..., MemorySectionName).
+  // Both name the same section's backing file in NT device form; if they ever
+  // diverged, every lookup here would miss and every module would look trusted.
   return aModules.Get(aModuleLoadInfo.mNtLoadInfo.mSectionName.AsString());
 }
 
@@ -971,6 +975,7 @@ UntrustedModulesProcessor::ProcessModuleLoadQueueChildProcess(
 
   nsTHashtable<nsStringCaseInsensitiveHashKey> alreadyAdded;
   ModuleIdentifiers moduleIdents;
+  uint32_t unverifiableLoads = 0;
 
   // Build the set of modules to be processed by the parent.
   for (UnprocessedModuleLoadInfoContainer* container : loadsToProcess) {
@@ -983,6 +988,9 @@ UntrustedModulesProcessor::ProcessModuleLoadQueueChildProcess(
 
     if (!entry.mNtLoadInfo.mSectionHandle) {
       // No section handle, so nothing the parent can verify.
+      if (entry.mNtLoadInfo.mSectionHandleUnavailable) {
+        ++unverifiableLoads;
+      }
       continue;
     }
 
@@ -993,6 +1001,9 @@ UntrustedModulesProcessor::ProcessModuleLoadQueueChildProcess(
 
     ipc::FileDescriptor section(entry.mNtLoadInfo.mSectionHandle.get());
     if (!section.IsValid()) {
+      // We had a handle but could not wrap it for IPC, which is the same kind
+      // of anomaly as failing to duplicate it.
+      ++unverifiableLoads;
       continue;
     }
 
@@ -1004,6 +1015,8 @@ UntrustedModulesProcessor::ProcessModuleLoadQueueChildProcess(
         NS_ERROR_ILLEGAL_DURING_SHUTDOWN, __func__);
   }
 
+  mProcessedModuleLoads.mUnverifiableLoads += unverifiableLoads;
+
   if (moduleIdents.IsEmpty()) {
     // Nothing to process
     return GetModulesTrustPromise::CreateAndResolve(Nothing(), __func__);
@@ -1068,9 +1081,11 @@ void UntrustedModulesProcessor::CompleteProcessing(
   ModulesMap& modules = aModulesAndLoads.mModMapResult.ref().mModules;
   const uint32_t& trustTestFailures =
       aModulesAndLoads.mModMapResult.ref().mTrustTestFailures;
+  const uint32_t& rejectedSections =
+      aModulesAndLoads.mModMapResult.ref().mRejectedSections;
   UnprocessedModuleLoads& loads = aModulesAndLoads.mLoads;
 
-  if (modules.IsEmpty() && !trustTestFailures) {
+  if (modules.IsEmpty() && !trustTestFailures && !rejectedSections) {
     // No data, nothing to save.
     return;
   }
@@ -1143,7 +1158,7 @@ void UntrustedModulesProcessor::CompleteProcessing(
   }
 
   if (processedStacks.empty() && processedEvents.isEmpty() &&
-      !sanitizationFailures && !trustTestFailures) {
+      !sanitizationFailures && !trustTestFailures && !rejectedSections) {
     // Nothing to save
     return;
   }
@@ -1161,6 +1176,7 @@ void UntrustedModulesProcessor::CompleteProcessing(
 
   mProcessedModuleLoads.mSanitizationFailures += sanitizationFailures;
   mProcessedModuleLoads.mTrustTestFailures += trustTestFailures;
+  mProcessedModuleLoads.mRejectedSections += rejectedSections;
 }
 
 // The thread priority of this job should match the priority that the child
@@ -1195,6 +1211,7 @@ RefPtr<ModulesTrustPromise> UntrustedModulesProcessor::GetModulesTrustInternal(
 
   ModulesMap& modMap = result.mModules;
   uint32_t& trustTestFailures = result.mTrustTestFailures;
+  uint32_t& rejectedSections = result.mRejectedSections;
 
   ModuleEvaluator modEval;
   MOZ_ASSERT(!!modEval);
@@ -1212,6 +1229,7 @@ RefPtr<ModulesTrustPromise> UntrustedModulesProcessor::GetModulesTrustInternal(
     nsAutoString resolvedNtPath;
     if (!ValidateAndResolveModuleSection(section, resolvedNtPath) ||
         resolvedNtPath.IsEmpty()) {
+      ++rejectedSections;
       continue;
     }
 

toolkit/xre/dllservices/tests/gtest/TestModuleFileValidation.cpp

@@ -13,6 +13,7 @@
 #include <aclapi.h>
 #include <sddl.h>
 
+#include "mozilla/FileUtilsWin.h"
 #include "mozilla/ipc/FileDescriptor.h"
 #include "mozilla/UntrustedModulesProcessor.h"
 #include "nsCOMPtr.h"
@@ -140,7 +141,7 @@ class ScopedModuleCopy final {
 
 }  // anonymous namespace
 
-TEST(TestModuleFileValidation, AcceptsLoadedModule)
+TEST(TestModuleFileValidation, AcceptsLoadedModuleAndVerifiesPathsMatch)
 {
   wchar_t xulPath[MAX_PATH + 1] = {};
   ASSERT_NE(::GetModuleFileNameW(::GetModuleHandleW(L"xul.dll"), xulPath,
@@ -158,10 +159,21 @@ TEST(TestModuleFileValidation, AcceptsLoadedModule)
   ASSERT_TRUE(fd.IsValid());
 
   nsAutoString resolved;
-  EXPECT_TRUE(ValidateAndResolveModuleSection(fd, resolved));
+  ASSERT_TRUE(ValidateAndResolveModuleSection(fd, resolved));
+
+  // CompleteProcessing looks up the parent's ModulesMap by this path, so it
+  // has to name the file the child loaded; if it named it differently, lookup
+  // would miss and every module would be mistakenly reported as trusted. It is
+  // in the NT device form that a child's loader observer records and that
+  // ModuleRecord expects, so it needs converting before it can be compared
+  // against a DOS path.
   EXPECT_TRUE(StringBeginsWith(resolved, u"\\Device\\"_ns));
-  EXPECT_TRUE(StringEndsWith(resolved, u"\\xul.dll"_ns,
-                             nsCaseInsensitiveStringComparator));
+
+  nsAutoString resolvedDosPath;
+  ASSERT_TRUE(NtPathToDosPath(resolved, resolvedDosPath));
+  EXPECT_TRUE(resolvedDosPath.Equals(path, nsCaseInsensitiveStringComparator))
+      << "resolved: " << NS_ConvertUTF16toUTF8(resolvedDosPath).get()
+      << ", expected: " << NS_ConvertUTF16toUTF8(path).get();
 }
 
 // An invalid descriptor must be refused rather than producing a path.

toolkit/xre/dllservices/tests/gtest/TestUntrustedModules.cpp

@@ -344,6 +344,10 @@ void UntrustedModulesFixture::ValidateUntrustedModules(
   }
   EXPECT_EQ(aData.mSanitizationFailures, 0U);
   EXPECT_EQ(aData.mTrustTestFailures, 0U);
+  // This fixture only exercises the parent process, which resolves its own
+  // module paths directly and so never goes through handle validation.
+  EXPECT_EQ(aData.mUnverifiableLoads, 0U);
+  EXPECT_EQ(aData.mRejectedSections, 0U);
 }
 
 BOOL CALLBACK UntrustedModulesFixture::InitialModuleLoadOnce(PINIT_ONCE, void*,
@@ -384,6 +388,7 @@ BOOL CALLBACK UntrustedModulesFixture::InitialModuleLoadOnce(PINIT_ONCE, void*,
   u"\"" TYPE u"\\." PID u"\":{" \
     u"\"processType\":\"" TYPE u"\",\"elapsed\":\\d+\\.\\d+," \
     u"\"sanitizationFailures\":0,\"trustTestFailures\":0," \
+    u"\"unverifiableLoads\":0,\"rejectedSections\":0," \
     u"\"events\":\\[{" \
       u"\"processUptimeMS\":\\d+,\"loadDurationMS\":\\d+\\.\\d+," \
       u"\"threadID\":\\d+,\"threadName\":\"Main Thread\"," \