1605478 - Data integrity problems in content process UntrustedModulesData

Status: RESOLVED FIXED

(Firefox :: Launcher Process, defect, P3)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Bug 1603714 was triggered because somehow we ended up with a content process sending UntrustedModulesData up to the parent where an event matched a non-existent module.

While I fixed the crash in that bug, unfortunately this condition still results in dropped third-party module pings from the affected machines.

We should add some diagnostics to learn why and correct this, so that we're not throwing away data that otherwise could be very useful.

Comment 1

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1605478 - Add temporary MOZ_DIAGNOSTIC_ASSERT to narrow down UntrustedModulesData's integrity problem. r=aklotz

Bug 1603714 showed there were UntrustedModulesData instances in which a load
event pointed to a module which did not exist in the modules list.

This patch adds MOZ_DIAGNOSTIC_ASSERT to the following places to narrow down
when it happened.

1. When processing load events
   1-1. [Content] UntrustedModulesProcessor::CompleteProcessing:
   Verify events of a trusted module were eliminated by GetModulesTrust
   1-2. [Content] UntrustedModulesData::AddNewLoads:
   Verify a new ModuleRecord matches the event
   1-3. [Content] UntrustedModulesProcessor::CompleteProcessing:
   Verify processed data after new items were appended.

2. When processed data is sent
   2-1. [Content] UntrustedModulesProcessor::GetAllProcessedData:
   Verify processed data before serialization.
   2-2. [Content] ParamTraits<mozilla::UntrustedModulesData>::WriteEvent:
   Verify processed data before transferring to the browser process
   2-3. [Browser] ParamTraits<mozilla::UntrustedModulesData>::ReadEvent:
   A final point to catch this integrity problem. We had an IPC error here.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/996f931d61b6
Add temporary MOZ_DIAGNOSTIC_ASSERT to narrow down UntrustedModulesData's integrity problem.  r=aklotz

Comment 3

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/996f931d61b6

Comment 4

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/6b1d5e72a65b
Backed out changeset 996f931d61b6 for build bustages in Unified_cpp_toolkit_xre0.obj

Comment 5

[Oana Pop-Rus]

Backed out changeset 996f931d61b6 (bug 1605478) for build bustages on Windows in Unified_cpp_toolkit_xre0.obj

This started to fail in today's central-as-beta simulations and would start to fail on beta tomorrow after central got merged to beta.

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=292157522&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=1351d08be2769309f449b5cc80e5c449e6d6caf2

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=292157522&repo=try&lineNumber=51272

Backout: https://hg.mozilla.org/mozilla-central/rev/6b1d5e72a65b5d97025a566196fa5cb63b7e4549

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2540a369a5a8
Add temporary MOZ_DIAGNOSTIC_ASSERT to narrow down UntrustedModulesData's integrity problem.  r=aklotz

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2540a369a5a8

Comment 9

[Toshihito Kikuchi [:toshi]]

We're getting data.
https://crash-stats.mozilla.org/search/?moz_crash_reason=%5EMOZ_DIAGNOSTIC_ASSERT&build_id=%3E%3D20200308095244&platform=Windows&version=75.0a1&date=%3E%3D2020-03-06T18%3A32%3A00.000Z&date=%3C2020-03-09T18%3A32%3A00.000Z&_facets=signature&_sort=build_id&_sort=moz_crash_reason&_sort=-date&_columns=date&_columns=signature&_columns=build_id&_columns=moz_crash_reason&_columns=uptime#crash-reports

All instances are hitting MOZ_DIAGNOSTIC_ASSERT(!event.IsTrusted()). Looking at raw dumps, the variable event points to xul.dll but event.mLoadDurationMS is empty. That's why it was not eliminated by the earlier check event.IsXULLoad().

Comment 10

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1605478 - Skip processing a loading event of xul.dll without duration. r=aklotz

Analyzing the diagnostics asserts that were temporarily introduced, there were
loading events of xul.dll where mLoadDurationMS was empty. We put them into
the array of loading events but excluding them from the array of modules,
resulting in serialization failure of UntrustedModulesData.

The crashing processes loaded external modules of antivirus application, such
as Symantec, AVAST, or Comodo. With these modules, xul.dll might be loaded
without hitting ntdll!LdrLoadDll (ModuleLoadInf::IsBare() == false).

The proposed fix is to skip processing a loading event of xul.dll regardless of
the state of mLoadDurationMS.

Comment 11

[Calixte Denizet (:calixte)]

There are 2 crashes with signature mozilla::UntrustedModulesData::VerifyConsistency, the moz_crash_reason is MOZ_DIAGNOSTIC_ASSERT(mModules.Get(evt.mModule->mResolvedNtName, nullptr)) (No match in the table):

• https://crash-stats.mozilla.com/report/index/fb906beb-4188-46aa-8d26-f11440200310
• https://crash-stats.mozilla.com/report/index/dc96ca3e-0c64-46ff-8554-cdca60200310

Comment 12

[Julien Cristau [:jcristau]]

Aaron, any chance you could review the patch here?

Comment 13

[Julien Cristau [:jcristau]]

Toshi, do we need separate bugs for the other assertions (comment 11)?

Comment 14

[Toshihito Kikuchi [:toshi]]

No need for now. I believe that signature is also caused by the same root-cause of MOZ_DIAGNOSTIC_ASSERT(!event.IsTrusted()).

My patch generates a random number several times, which is used to reduce the number of affected users. The case of MOZ_DIAGNOSTIC_ASSERT(mModules.Get(evt.mModule->mResolvedNtName, nullptr)) and MOZ_DIAGNOSTIC_ASSERT(false) (Something else) happens when a first random number was lucky enough to pass through the check, but after that, a second or a third random number was not.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e87ab6d3d9ab
Skip processing a loading event of xul.dll without duration.  r=aklotz

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e87ab6d3d9ab

Comment 17

[Toshihito Kikuchi [:toshi]]

Seems like all of the three diagnostics asserts are gone after the fix. We can back out the asserts.

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset e87ab6d3d9ab (Bug 1605478) on toshi's request

Backout link: https://hg.mozilla.org/integration/autoland/rev/250960afe914203f41054e25728407476cd938fe

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab2014f6c4af
Skip processing a loading event of xul.dll without duration.  r=aklotz
https://hg.mozilla.org/integration/autoland/rev/93abde6b396f
Backed out changeset 2540a369a5a8 on toshi's request CLOSED TREE

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

Sorry for the miss match.

Comment 21

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/ab2014f6c4af