I'd like to add metadata for the company whose cert was used to sign a DLL, if it is unknown to us. This would help expedite the identification of unknown injected DLLs.

Can we add this info to the modules ping too? The ping can give a clearer picture of the entire population, while the crash data is only about people who crash and choose to submit a report.

Where is the modules ping? Is that in telemetry?

(In reply to Aaron Klotz [:aklotz] from comment #2) > Where is the modules ping? Is that in telemetry? Yes, it was added in bug 1330833. The relevant code is in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#826 and https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryModules.jsm. Here's the documentation of the ping: https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/docs/data/modules-ping.rst.

I'll file a followup bug for the modules ping.

Comment on attachment 8946953 [details] Data Review Request Form That looks good. Just one that could make the documentation (i.e. the attached Data Review Request file) clearer. Can you give examples of what the collected data will look like? Is it a string like "JAWS" or "Adobe"? Do you know if the schema for crash reports is documented anywhere? Just trying to figure out if there's documentation somewhere that will need to be amended with this extra field.

Comment on attachment 8946937 [details] Bug 1430857: Part 1 - Refactor DllServices to make it possible to obtain them from anywhere in Gecko; https://reviewboard.mozilla.org/r/216782/#review222846 lgtm, mostly code shifting around.

Comment on attachment 8946938 [details] Bug 1430857: Part 2 - Add cert annotations to Windows crash reports; https://reviewboard.mozilla.org/r/216784/#review222754 Looks great overall; the Wintrust/crypt code specifically is good, but there's a couple of other details I don't quite understand, and I need to before I can r+. ::: mozglue/build/Authenticode.h:15 (Diff revision 1) > +#include "mozilla/Maybe.h" > +#include "mozilla/UniquePtr.h" > + > +namespace mozilla { > + > +class Authenticode I need some help understanding what this Authenticode interface is all about. I don't see what it is that we're getting out of having an interface and a singleton that implements it, which then has to get managed by the DLL service, as opposed to just a loose GetBinaryOrgName function. Especially since the AuthenticodeImpl object is stateless. ::: mozglue/build/Authenticode.cpp:152 (Diff revision 1) > + trustData.dwUnionChoice = WTD_CHOICE_FILE; > + trustData.pFile = &fileInfo; > + trustData.dwStateAction = WTD_STATEACTION_VERIFY; > + > + GUID policyGUID = WINTRUST_ACTION_GENERIC_VERIFY_V2; > + LONG result = WinVerifyTrust(nullptr, &policyGUID, &trustData); nit: If we're being pedantic, MSDN would have us pass INVALID_HANDLE_VALUE instead of nullptr, to signal that we're not okay with the trust provider showing any UI to the user during this call. I doubt it would ever make any difference, but no harm in being careful. ::: toolkit/crashreporter/CertAnnotator.h:39 (Diff revision 1) > + void RecordCertInfo(const nsAString& aLibPath, const bool aDoSerialize); > + > + void Serialize(); > + void Annotate(const nsCString& aAnnotation); > + > + nsClassHashtable<nsStringHashKey, nsTHashtable<nsStringHashKey>> mCertTable; Using another hashtable as the value type for mCertTable threw me off for a while, until I figured out we're inserting just keys into it without any entries, to sort of emulate a hashset structure. Could an nsTArray be used here instead? That would make it more intuitive. If not, a comment explaining why it's how it is wouldn't go amiss. ::: toolkit/crashreporter/CertAnnotator.cpp:148 (Diff revision 1) > + > +} // namespace mozilla > + > +namespace { > + > +class Writer final : public mozilla::JSONWriteFunc This is at least the fifth separate implementation in our tree of a JSONWriteFunc that writes to a string. Not your problem to solve, but still, :(

Comment on attachment 8946938 [details] Bug 1430857: Part 2 - Add cert annotations to Windows crash reports; https://reviewboard.mozilla.org/r/216784/#review222754 > I need some help understanding what this Authenticode interface is all about. I don't see what it is that we're getting out of having an interface and a singleton that implements it, which then has to get managed by the DLL service, as opposed to just a loose GetBinaryOrgName function. Especially since the AuthenticodeImpl object is stateless. The main reasoning is because I don't want this interface to be exported from mozglue.dll, as this function is going to be a critical part of our DLL blocking infrastruture. Yes, I know that since we're in user mode, we're essentially in an arms race with our adversaries. But I'd rather not make it easier for them to mess with that function than it needs to be. > Using another hashtable as the value type for mCertTable threw me off for a while, until I figured out we're inserting just keys into it without any entries, to sort of emulate a hashset structure. > Could an nsTArray be used here instead? That would make it more intuitive. If not, a comment explaining why it's how it is wouldn't go amiss. Sure. Initially my concern was that the list of modules could become fairly large, but really I only expect that in one case (The Windows Cert). I think we can use a sorted nsTArray instead.

(In reply to François Marier [:francois] from comment #8) > Comment on attachment 8946953 [details] > Data Review Request Form > Can you give examples of what the collected data will look like? Is it a > string like "JAWS" or "Adobe"? > > Do you know if the schema for crash reports is documented anywhere? Just > trying to figure out if there's documentation somewhere that will need to be > amended with this extra field. I have added some additional notes under section (5) in the updated request document that should help answer these questions.

Comment on attachment 8946938 [details] Bug 1430857: Part 2 - Add cert annotations to Windows crash reports; https://reviewboard.mozilla.org/r/216784/#review222754 > The main reasoning is because I don't want this interface to be exported from mozglue.dll, as this function is going to be a critical part of our DLL blocking infrastruture. > > Yes, I know that since we're in user mode, we're essentially in an arms race with our adversaries. But I'd rather not make it easier for them to mess with that function than it needs to be. Works for me; I thought you must have had a reason and I just wasn't getting it.

Comment on attachment 8946938 [details] Bug 1430857: Part 2 - Add cert annotations to Windows crash reports; https://reviewboard.mozilla.org/r/216784/#review223790

Comment on attachment 8948557 [details] Data Review Request Form (r2) 1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate? Yes, attached to this bug. 2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available. Yes: crash report and telemetry settings. 3) If the request is for permanent data collection, is there someone who will monitor the data over time?** Yes, Aaron. 4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under? ** Category 1. 5) Is the data collection request for default-on or default-off? Default-on 6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)? No. 7) Is the data collection covered by the existing Firefox privacy notice? Yes. 8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)** No, permanent.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s e3d158cc92f3cd9479b4901993141ae7070dead0 -d 87b9333134d1: rebasing 446032:e3d158cc92f3 "Bug 1430857: Part 1 - Refactor DllServices to make it possible to obtain them from anywhere in Gecko; r=jimm" merging mozglue/build/moz.build merging toolkit/xre/nsAppRunner.cpp warning: conflicts while merging mozglue/build/moz.build! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by aklotz@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6b88557d1e50 Part 1 - Refactor DllServices to make it possible to obtain them from anywhere in Gecko; r=jimm https://hg.mozilla.org/integration/autoland/rev/b12ea04f9c5a Part 2 - Add cert annotations to Windows crash reports; r=mhowell

Backout by rgurzau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7a990bbf6046 Backed out 2 changesets for build bustage on a CLOSED TREE

Backed out 2 changesets (bug 1430857) for build bustage on a CLOSED TREE Backout link: https://hg.mozilla.org/integration/autoland/rev/7a990bbf6046fc2332a6ba95a376bdd6f84d909b Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b12ea04f9c5ae5626fda1968e5c1cbc98bdebcb6 Log: https://treeherder.mozilla.org/logviewer.html#?job_id=160676090&repo=autoland&lineNumber=21786 [task 2018-02-06T21:15:25.610Z] 21:15:25 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/toolkit/crashreporter/Unified_cpp_crashreporter0.cpp:11:0: [task 2018-02-06T21:15:25.610Z] 21:15:25 INFO - /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp:94:41: fatal error: mozilla/WindowsDllBlocklist.h: No such file or directory [task 2018-02-06T21:15:25.610Z] 21:15:25 INFO - #include "mozilla/WindowsDllBlocklist.h" [task 2018-02-06T21:15:25.611Z] 21:15:25 INFO - ^ [task 2018-02-06T21:15:25.611Z] 21:15:25 INFO - compilation terminated. [task 2018-02-06T21:15:25.611Z] 21:15:25 INFO - /builds/worker/workspace/build/src/config/rules.mk:1047: recipe for target 'Unified_cpp_crashreporter0.o' failed [task 2018-02-06T21:15:25.611Z] 21:15:25 INFO - make[4]: *** [Unified_cpp_crashreporter0.o] Error 1 [task 2018-02-06T21:15:25.611Z] 21:15:25 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/crashreporter' [task 2018-02-06T21:15:25.611Z] 21:15:25 INFO - /builds/worker/workspace/build/src/config/recurse.mk:73: recipe for target 'toolkit/crashreporter/target' failed [task 2018-02-06T21:15:25.611Z] 21:15:25 INFO - make[3]: *** [toolkit/crashreporter/target] Error 2 [task 2018-02-06T21:15:25.611Z] 21:15:25 INFO - make[3]: *** Waiting for unfinished jobs....

Just some minor include fixes were needed.

Pushed by aklotz@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4bdd6d82f993 Part 1 - Refactor DllServices to make it possible to obtain them from anywhere in Gecko; r=jimm https://hg.mozilla.org/integration/autoland/rev/cc9b0ac5f66b Part 2 - Add cert annotations to Windows crash reports; r=mhowell

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/a992887a6060 Follow-up - Fix mingw header inclusion failure; r=bustage

https://hg.mozilla.org/mozilla-central/rev/4bdd6d82f993 https://hg.mozilla.org/mozilla-central/rev/cc9b0ac5f66b https://hg.mozilla.org/mozilla-central/rev/a992887a6060

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/f3238e5b131b Port bug 1430857 to TB/IB/SM: move include of WindowsDllBlocklist.h in mainline. rs=bustage-fix

I think this is responsible for breaking the Windows code coverage build, I see crashes @ arena_dalloc with CertAnnotator on the stack.

Backed out 3 changesets (bug 1430857) for breaking tests on Windows Code Coverage builds a=backout https://treeherder.mozilla.org/logviewer.html#?job_id=160807480&repo=mozilla-central&lineNumber=1171 https://hg.mozilla.org/mozilla-central/rev/ea00596eb02e86a919c6734a2307ff118a01d257

https://hg.mozilla.org/comm-central/rev/1c655004b4e20713f536a478581a5bab52a8fe7b Backed out changeset f3238e5b131b (comm-central part of bug 1430857) since bug 1430857 was backed out. a=backout

I think, sadly, that this might be related to the fact that we allocate the cert subject buffer from within mozglue but free it from within xul. They're both using jemalloc, but perhaps there is a subtle disconnect between malloc in mozglue and free in xul.

I don't know why, but mozregression for bug 1436351 brings me to this bug. I just started seeing that today, but the reporter says "from a few days ago", so is could be wrong I suppose.

Yeah, that's definitely not from this bug.

I think this is related to the malloc arenas created when NIGHTLY_BUILD is defined.

Pushed by aklotz@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/75b888356209 Part 1 - Refactor DllServices to make it possible to obtain them from anywhere in Gecko; r=jimm https://hg.mozilla.org/integration/autoland/rev/828fdcd72a67 Part 2 - Add cert annotations to Windows crash reports; r=mhowell

This should be fixed now. In Authenticode.cpp, operator new was calling the CRT malloc on coverage builds.

https://hg.mozilla.org/mozilla-central/rev/75b888356209 https://hg.mozilla.org/mozilla-central/rev/828fdcd72a67