Closed Bug 1430857 Opened 6 years ago Closed 6 years ago

Include authenticode cert information with crash reports

Categories

(Toolkit :: Crash Reporting, enhancement)

Product:
Toolkit
Component:
Crash Reporting
Platform:
Unspecified
Windows
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: bugzilla, Assigned: bugzilla)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1430857: Part 1 - Refactor DllServices to make it possible to obtain them from anywhere in Gecko;
59 bytes, text/x-review-board-request
jimm
: review+
Details
Bug 1430857: Part 2 - Add cert annotations to Windows crash reports;
59 bytes, text/x-review-board-request
molly
: review+
Details
Data Review Request Form (r2)
3.77 KB, text/plain
francois
: review+
Details
Follow-up: Fix mingw stupidity
882 bytes, patch
Details | Diff | Splinter Review 
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?
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)
I'll file a followup bug for the modules ping.
Attached file Data Review Request Form (obsolete) — 

        Attachment #8946953 -
        Flags: review?(francois)
Summary: Include authenticode cert information with crash reports containing unknown DLLs → Include authenticode cert information with crash reports

    
    

    
  
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 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 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, :(
Blocks: 1435773
No longer blocks: injecteject

    
    

    
      
  

    
    

    
  
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.

        Attachment #8946953 -
        Attachment is obsolete: true

        Attachment #8948557 -
        Flags: review?(francois)

    
    

    
  
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

        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)

    
    

    
      
  

    
    

    
      
  

    
    

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

    
    

    
      
  

    
    

    
      
  

    
    

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

    
    

    
  
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox60:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

    
    

    
  
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 → ---

    
    

    
  
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.
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.

    
    

    
  
Yeah, that's definitely not from this bug.

    
    

    
  
I think this is related to the malloc arenas created when NIGHTLY_BUILD is defined.
No longer blocks: 1436351

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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.
Depends on: 1436684

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/75b888356209
https://hg.mozilla.org/mozilla-central/rev/828fdcd72a67
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago

          status-firefox60:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1436845
See Also:  → 1436860

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: