Include authenticode cert information with crash reports

RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla60
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 2

a year ago
Where is the modules ping? Is that in telemetry?
Flags: needinfo?(mcastelluccio)
(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.
Flags: needinfo?(mcastelluccio)
(Assignee)

Comment 4

a year ago
I'll file a followup bug for the modules ping.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1434489
(Assignee)

Comment 7

a year ago
Posted file Data Review Request Form (obsolete) —
Attachment #8946953 - Flags: review?(francois)
(Assignee)

Updated

a year ago
Summary: Include authenticode cert information with crash reports containing unknown DLLs → Include authenticode cert information with crash reports
(Assignee)

Updated

a year ago
Blocks: 1434495
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.
Attachment #8946953 - Flags: review?(francois)

Comment 9

a year ago
mozreview-review
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.
Attachment #8946937 - Flags: review?(jmathies) → review+

Comment 10

a year ago
mozreview-review
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, :(
(Assignee)

Updated

a year ago
Blocks: 1435773
No longer blocks: 1306406
(Assignee)

Updated

a year ago
Blocks: 1435793
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 13

a year ago
(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.
Attachment #8946953 - Attachment is obsolete: true
Attachment #8948557 - Flags: review?(francois)

Comment 14

a year ago
mozreview-review-reply
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 15

a year ago
mozreview-review
Comment on attachment 8946938 [details]
Bug 1430857: Part 2 - Add cert annotations to Windows crash reports;

https://reviewboard.mozilla.org/r/216784/#review223790
Attachment #8946938 - Flags: review?(mhowell) → review+
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.
Attachment #8948557 - Flags: review?(francois) → review+
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

a year ago
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

Comment 21

a year ago
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....
Flags: needinfo?(aklotz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

a year ago
Just some minor include fixes were needed.
Flags: needinfo?(aklotz)

Comment 27

a year ago
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

Comment 29

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

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4bdd6d82f993
https://hg.mozilla.org/mozilla-central/rev/cc9b0ac5f66b
https://hg.mozilla.org/mozilla-central/rev/a992887a6060
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

Comment 31

a year ago
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.
Status: RESOLVED → REOPENED
status-firefox60: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---

Comment 34

a year ago
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
(Assignee)

Comment 35

a year ago
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.
Flags: needinfo?(aklotz)
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.
(Assignee)

Comment 37

a year ago
Yeah, that's definitely not from this bug.
(Assignee)

Comment 38

a year ago
I think this is related to the malloc arenas created when NIGHTLY_BUILD is defined.
(Assignee)

Updated

a year ago
No longer blocks: 1436351
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 42

a year ago
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
(Assignee)

Comment 43

a year ago
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
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Assignee)

Updated

a year ago
Depends on: 1436845
See Also: → bug 1436860
You need to log in before you can comment on or make changes to this bug.